public inbox for [email protected]
help / color / mirror / Atom feedFrom: Akshay Joshi <[email protected]>
To: Nikhil Mohite <[email protected]>
Cc: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL
Date: Tue, 11 May 2021 13:32:42 +0530
Message-ID: <CANxoLDd2xMu7ym9PFdhYUSy4ucNuPoZ2fvaRkuwyB+EqowLXwA@mail.gmail.com> (raw)
In-Reply-To: <CAOBg0AMVyLBma8wsbZ-VQWF4q3OAhqWeRLpmrxVcM_YOKH+MDQ@mail.gmail.com>
References: <CAOBg0AO6Tjksb+EOA_-O13yRzdL_2d3y6G74m=GGQS+Ymjv=0g@mail.gmail.com>
<CA+OCxow9+UDMrsBBvJKW2AeAYaN4cO3w7kBGNKtsWhSM6_D7gQ@mail.gmail.com>
<CAOBg0AMVyLBma8wsbZ-VQWF4q3OAhqWeRLpmrxVcM_YOKH+MDQ@mail.gmail.com>
Hi Nikhil
Following are the review comments:
*GUI specific*:
- We need a panel icon for PSQL like query tool, we can also add that on
the browser tree toolbar.
- PSQL Tool menu should be visible for all the child nodes of the
database node. Follow the same as Query Tool.
- PSQL tab title should be only database server name as the user can
change the database/user from PSQL command, so it's been difficult to
update the tab title.
- PSQL connection is still open even if we disconnect the database
server from the browser tree.
*Code specific:*
- Remove an extra space from requirements.txt and package.json
- Documentation needs to be updated to let the user know from where the
PSQL tool will open and on which node it is applicable.
- psql/__init__.py check there are so many unused imports please remove
them.
- We are not using cheroot so it should be removed from requirements.txt
and also remove the import statement from pgAdmin4.py
- Test cases are showing successful but actually, there are some routing
errors please check.
On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite <
[email protected]> wrote:
> Hi Dave/ Team,
>
> PFA updated patch, sorry for the inconvenience, while cleanup I removed
> the unwanted libraries but forgot to remove the code related to them.
>
> On Mon, May 10, 2021 at 7:10 PM Dave Page <[email protected]> wrote:
>
>> Hi
>>
>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>> [email protected]> wrote:
>>
>>> Hi Hackers,
>>>
>>> Please find the attached patch for RM-2341
>>> <https://redmine.postgresql.org/issues/2341;: Add Menu option for
>>> starting PSQL.
>>> 1. Added new Option PSQL Tool in Tools menu.
>>> 2. Added the same option for Server and Database nodes from the tree
>>> view.
>>>
>>
>> Unfortunately there's a trailing comma in package.json that makes it
>> invalid. If I fix that, then I get the error below, so I'm guessing the
>> intention was to actually include another package there?
>>
>> ERROR in ./pgadmin/tools/psql/static/js/psql_module.js 23:50-82
>> Module not found: Error: Can't resolve 'local-echo-controller' in
>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>> resolve 'local-echo-controller' in
>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>> Parsed request is a module
>> using description file: /Users/dpage/git/pgadmin4/web/package.json
>> (relative path: ./pgadmin/tools/psql/static/js)
>> aliased with mapping 'local-echo-controller':
>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' to
>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo'
>> using description file: /Users/dpage/git/pgadmin4/web/package.json
>> (relative path: ./pgadmin/tools/psql/static/js)
>> Field 'browser' doesn't contain a valid alias configuration
>> root path /Users/dpage/git/pgadmin4/web
>> using description file:
>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>> ./Users/dpage/git/pgadmin4/web/node_modules/local-echo)
>> no extension
>> Field 'browser' doesn't contain a valid alias configuration
>>
>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>> doesn't exist
>> .js
>> Field 'browser' doesn't contain a valid alias configuration
>>
>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.js
>> doesn't exist
>> .jsx
>> Field 'browser' doesn't contain a valid alias configuration
>>
>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx
>> doesn't exist
>> as directory
>>
>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>> doesn't exist
>> using description file:
>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>> ./node_modules/local-echo)
>> no extension
>> Field 'browser' doesn't contain a valid alias configuration
>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't
>> exist
>> .js
>> Field 'browser' doesn't contain a valid alias configuration
>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.js
>> doesn't exist
>> .jsx
>> Field 'browser' doesn't contain a valid alias configuration
>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx
>> doesn't exist
>> as directory
>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't
>> exist
>> @ ./pgadmin/tools/psql/static/js/index.js 17:19-43
>>
>> 2021-05-10 14:38:37: webpack 5.21.2 compiled with 1 error in 60041 ms
>>
>> --
>> Dave Page
>> Blog: https://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EDB: https://www.enterprisedb.com
>>
>
> Regards,
> Nikhil Mohite
>
--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*
*Mobile: +91 976-788-8246*
view thread (54+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL
In-Reply-To: <CANxoLDd2xMu7ym9PFdhYUSy4ucNuPoZ2fvaRkuwyB+EqowLXwA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox