Hi
On Tue, Jun 6, 2017 at 9:15 PM, Sarah McAlear <smcal...@pivotal.io> wrote:
> Hello,
>
>> First patch contains changes related to copy row feature which is already
>> in process in another thread. Any reason to add those changes in this patch
>> as this patch is meant to contain infrastructure changes ?
>
>
> As part of introducing React, we are bringing a linter into the codebase.
> The changes to the file you're referring to are formatting changes enforced
> by the linter.
Right - but they also make it a pain to review the code and see what
has actually changed vs. what the linter has reformatted. Can we
separate those changes out easily?
>> Can the changes related to FeatureTest or Jasmine test be separated out in
>> another patch?
>
>
> Great question! Since we test-drove this, the value delivered by each
> individual test represents a behavior. Each piece of functionality in the
> implementation is directly related to their corresponding test(s).
> In this way, the tests belong with the code in the same way as
> documentation.
I totally agree with that. We should always strive to have code, docs
and tests together.
> The new 2 patches are the same as before, without Grunt.
Unfortunately they don't apply cleanly, and break the feature tests:
With the first patch only:
- The patch includes yarn.lock, which is a gitignore'd file.
- karma.conf.js needed manual merging of every hunk.
- When the feature tests run, the browser just flickers whilst it
continually tries to connect to the server, all the time getting
ERR_CONNECTION_REFUSED.
Patch number 2 fails to apply on karma.conf.js, sqleditor.js,
pgadmin_page.py and yarn.lock (which shouldn't be there anyway).
Can you fix/rebase please?
> On Mon, Jun 5, 2017 at 1:30 AM, Surinder Kumar
> <surinder.ku...@enterprisedb.com> wrote:
>>
>> Hi
>>
>> Review comments:
>>
>> 1) First patch contains changes related to copy row feature which is
>> already in process in another thread. Any reason to add those changes in
>> this patch as this patch is meant to contain infrastructure changes ?
>>
>> 2) Can the changes related to FeatureTest or Jasmine test be separated out
>> in another patch?
>>
>>
>> Thanks,
>> Surinder
>>
>>
>> On Thu, Jun 1, 2017 at 1:59 AM, Matthew Kleiman <mklei...@pivotal.io>
>> wrote:
>>>
>>> Hey Surinder,
>>>
>>>> Is it possible to run the task listed above by configuring them in
>>>> "scripts" in package.json file and running concurrently ?
>>>
>>>
>>> That would be cleaner than what we proposed. We could use the yarn run
>>> feature to do just what you are suggesting. It works the same as in npm.
>>> Something like the following:
>>> In package.json -
>>> "scripts": {
>>> "linter": "eslint pgadmin/static/jsx/**/*.jsx
>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
>>> regression/javascript/**/*.js *.js"
>>> }
>>>
>>> To call it on the command line -
>>> yarn run linter
>>>
>>> - Matt
>>>
>>> On Wed, May 31, 2017 at 12:35 AM, Surinder Kumar
>>> <surinder.ku...@enterprisedb.com> wrote:
>>>>
>>>> On Wed, May 31, 2017 at 12:01 AM, Joao Pedro De Almeida Pereira
>>>> <jdealmeidapere...@pivotal.io> wrote:
>>>>>>
>>>>>> The motivation is simple - we want a solution that works for the whole
>>>>>> app, can handle debug vs. release execution, pluggable modules, and
>>>>>> installations in read-only directories.
>>>>>
>>>>> With the current configuration of Grunt, all the requirements you
>>>>> mention are available.
>>>>> The tasks on Grunt should only be run under development or before we
>>>>> are creating the installer.
>>>>
>>>>
>>>>>
>>>>> The installer should pick up only the bundled Javascript and should
>>>>> install it in the correct folders, this way there is no need to rewrite or
>>>>> process files in read-only directories of the end user machine.
>>>>>
>>>>>> Per the other thread on the subject (that Joao suggested continuing
>>>>>> discussion on), Surinder is currently looking into flask-webpack. I
>>>>>> spent some time playing with grunt and some other options last week.
>>>>>
>>>>> We should continue the discussion about flask-webpack or Grunt or any
>>>>> other Build system in the other thread, but we need to have some decision
>>>>> because this patch needs a build system.
>>>>> If we remove Grunt from this patch we will need to execute the
>>>>> following command every time the application run:
>>>>>
>>>>> yarn run eslint pgadmin/static/jsx/**/*.jsx
>>>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
>>>>> regression/javascript/**/*.js *.js && yarn run webpack -- --config
>>>>> webpack.config.js && python web/pgAdmin4
>>>>>
>>>>> And to run the jasmine tests:
>>>>>
>>>>> yarn run eslint pgadmin/static/jsx/**/*.jsx
>>>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
>>>>> regression/javascript/**/*.js *.js && yarn run karma start
>>>>>
>>>>> And to run the feature tests:
>>>>>
>>>>> yarn run eslint pgadmin/static/jsx/**/*.jsx
>>>>> pgadmin/static/js/selection/*.js regression/javascript/**/*.jsx
>>>>> regression/javascript/**/*.js *.js && yarn run webpack -- --config
>>>>> webpack.config.js && python runtests.py
>>>>>
>>>>> Is this a solution that is acceptable?
>>>>
>>>>
>>>> As per my knowledge, Grunt can run the tasks and bundle the JS and CSS
>>>> files (using r.js which is not preferred). But Webpack can not do both. It
>>>> can only bundle files, it has to be used with either Grunt or Gulp to
>>>> execute tasks.
>>>>
>>>> Is it possible to run the task listed above by configuring them in
>>>> "scripts" in package.json file and running concurrently ?
>>>>
>>>> scripts: {
>>>> "task-name1": "yarn run eslint filename",
>>>> "task-name2": "yarn run karma start", so on...
>>>> }
>>>>
>>>> and run them using following command:
>>>> npm run task-name1
>>>>
>>>>>
>>>>>
>>>>>> However; this patch is supposed to be about the history tab rewrite.
>>>>>> Whatever solution we use for webpacking/transpiling/linting/minifying
>>>>>> etc, it should be a standalone change as it's decidedly non-trivial.
>>>>>
>>>>> We split this patch into 2 different commits:
>>>>> 1 - Add React and all the tools needed to work with it, this includes
>>>>> Grunt, Webpack and ESLint
>>>>> 2 - Change the history tab
>>>>>
>>>>> Do you want a single commit for each of the following?
>>>>> React (just adds the React library via yarn)
>>>>> Webpack (adds Webpack library via yarn and creates the
>>>>> webpack.config.js)
>>>>> ESLint (adds ESLint library via yarn, creates .eslintrc config file,
>>>>> and corrects all lint errors that previously existed)
>>>>> Grunt (adds Grunt library via yarn and creates Gruntfile.js config,
>>>>> creating a pipeline of the previous three libraries/tasks)
>>>>> Our change to History
>>>>>
>>>>> Thanks
>>>>> Joao & Matt
>>>>>
>>>>> On Tue, May 30, 2017 at 10:10 AM, Dave Page <dp...@pgadmin.org> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Tue, May 30, 2017 at 2:47 PM, Matthew Kleiman <mklei...@pivotal.io>
>>>>>> wrote:
>>>>>> > Hi Dave,
>>>>>> >
>>>>>> > We are currently using the Grunt taskrunner to run the following
>>>>>> > tasks:
>>>>>> >
>>>>>> > lint the javascript code
>>>>>> > start javascript tests
>>>>>> > invoke webpack to transpile and bundle js and jsx files
>>>>>> > minify javascript
>>>>>> >
>>>>>> > In order to remove Grunt from the application, we will need some
>>>>>> > other tool
>>>>>> > to execute these listed tasks.
>>>>>> >
>>>>>> > There are other tools available, like Gulp.js, that we could use
>>>>>> > instead of
>>>>>> > Grunt. We would like to understand your motivation for removing
>>>>>> > Grunt so we
>>>>>> > can find a better solution.
>>>>>>
>>>>>> The motivation is simple - we want a solution that works for the whole
>>>>>> app, can handle debug vs. release execution, pluggable modules, and
>>>>>> installations in read-only directories.
>>>>>>
>>>>>> Per the other thread on the subject (that Joao suggested continuing
>>>>>> discussion on), Surinder is currently looking into flask-webpack. I
>>>>>> spent some time playing with grunt and some other options last week.
>>>>>>
>>>>>> However; this patch is supposed to be about the history tab rewrite.
>>>>>> Whatever solution we use for webpacking/transpiling/linting/minifying
>>>>>> etc, it should be a standalone change as it's decidedly non-trivial.
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>>
>>>>
>>>
>>
>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
