Re: [pgAdmin4] [PATCH] History Tab rewrite in React

From: George Gelashvili <ggelashvili(at)pivotal(dot)io>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Sarah McAlear <smcalear(at)pivotal(dot)io>, Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com>, Matthew Kleiman <mkleiman(at)pivotal(dot)io>, Joao Pedro De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>, Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4] [PATCH] History Tab rewrite in React
Date: 2017-06-10 01:52:12
Message-ID: CAHowoHZFkDQrMO0-e2gx3ffo3kB56Qwb3jT2S1M7Sc8Bpo0Usw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi Dave,

Our patch touches code also changed by the patches that were recently
committed.
That's likely what's causing this issue. We've rebased on top of the new
state of master.

We had initially kept the yarn.lock .gitignored, but ran into an issue
rather early on where an upgraded dependency introduced a regression.
Checking-in the yarn.lock provides the "know your dependency version"
benefit of vendorizing code without vendorization's drawback of having to
manually manage your dependencies.

It is safe to delete a yarn.lock before applying a patch, as you are
authoring master. It provides a history of the versions of each dependency
that were working at the point in time of the commit. By contrast,
package.json provides approximate versions and leaves room for
fixes/improvements by the dependency authors to be pulled in as they become
available.

*To run linter and tests:*

The tasks that Grunt used to manage are now defined as a set of scripts in
the package.json
After applying the patches--which may require deleting yarn.lock for the
first patch--you should cd web && yarn install

Then yarn test will run the linter, jasmine specs, and python tests
including feature tests, in that order, exiting early if there are
failures/errors.
At the moment, the CheckForViewData test is failing on master as well as in
each of these patches; that should be resolved as RM2477.

Thanks!
George and Matt

On Thu, Jun 8, 2017 at 9:15 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi George
>
> On Wed, Jun 7, 2017 at 10:21 PM, George Gelashvili
> <ggelashvili(at)pivotal(dot)io> wrote:
> > Hi Dave,
> >
> > I split the linting out into an intermediate commit, and rebased on top
> of
> > master.
>
> Unfortunately, it still doesn't apply:
>
> error: patch failed: web/regression/javascript/test-main.js:1
> error: removal patch leaves file contents
> error: web/regression/javascript/test-main.js: patch does not apply
> Checking patch web/regression/requirements.txt...
> Checking patch web/webpack.config.js...
> Checking patch web/webpack.test.config.js...
> Checking patch web/yarn.lock...
> error: web/yarn.lock: already exists in working directory
> Applied patch .gitignore cleanly.
> Applied patch Make.bat cleanly.
> Applied patch README cleanly.
> Applied patch pkg/mac/build.sh cleanly.
> Applied patch pkg/pip/build.sh cleanly.
> Applied patch pkg/src/build.sh cleanly.
> Applied patch web/.eslintrc.js cleanly.
> Applied patch web/karma.conf.js cleanly.
> Applied patch web/package.json cleanly.
> Applied patch web/pgAdmin4.py cleanly.
> Applied patch web/pgadmin/static/jsx/components.jsx cleanly.
> Applied patch web/pgadmin/tools/javascript/__init__.py cleanly.
> Applied patch web/pgadmin/tools/javascript/javascript_bundler.py cleanly.
> Applied patch web/pgadmin/tools/javascript/tests/__init__.py cleanly.
> Applied patch web/pgadmin/tools/javascript/tests/test_javascript_bundler.
> py
> cleanly.
> Applied patch web/regression/README cleanly.
> Applied patch web/regression/javascript/jasmine_capture_warnings_
> beforeall.js
> cleanly.
> Applied patch web/regression/requirements.txt cleanly.
> Applied patch web/webpack.config.js cleanly.
> Applied patch web/webpack.test.config.js cleanly.
>
> The second (lint update) patch is even worse, with significant number
> change that just don't want to apply.
>
> Clearly yarn.lock needs to be removed from there repo.
>
> Once I can apply a version of this, how should I be running the linter
> and the unit tests?
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Attachment Content-Type Size
01-start-tracking-yarn-lock.diff text/plain 317 bytes
02-bring-react-into-the-codebase.diff text/plain 319.1 KB
03-lint.diff text/plain 68.8 KB
04-create-query-history-list.diff text/plain 43.9 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Dave Page 2017-06-11 12:56:55 pgAdmin 4 commit: Couple of minor performance tweaks. Tests on my Windo
Previous Message Shirley Wang 2017-06-09 13:05:50 Re: [patch] Changing the ACI tree font to Helvetica