Re: [GSoC][Patch] Automatic Mode Detection V1

Hi

[please keep the maililng list CC'd]

On Mon, Jun 17, 2019 at 3:05 PM Yosry Muhammad <yosrym93@gmail.com> wrote:

Do you want me to ask our design guy for an icon?

That would be great to keep things clear and separated for the users.

I've asked Chethana (CC'd) to create one.

 

Please find attached a patch to fix the problem that happened with you. The problem is that I edited the primary_keys.sql file in web/tools/sqleditor/templates/sqleditor/sql/default/ only, while there was another one in ..../templates/sqleditor/sql/11_plus/. I wonder what happens with versions before 11? are the scripts in the default/ folder used if they are not found in that version folder?

The patch also removes a few unnecessary lines of code that I found, not related to the problem.

Ahh, yes - that works :-). I haven't done a detailed code review yet as you're going to be whacking things around for a bit, but I didn't see any obvious styling issues except for:

(pgadmin4) dpage@hal:~/git/pgadmin4$ make check-pep8
pycodestyle --config=.pycodestyle docs/
pycodestyle --config=.pycodestyle pkg/
pycodestyle --config=.pycodestyle web/
web/pgadmin/tools/sqleditor/__init__.py:440: [E125] continuation line with same indent as next logical line
web/pgadmin/tools/sqleditor/command.py:929: [E501] line too long (80 > 79 characters)
web/pgadmin/tools/sqleditor/command.py:977: [W391] blank line at end of file
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:53: [E501] line too long (92 > 79 characters)
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:74: [E501] line too long (80 > 79 characters)
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:81: [E501] line too long (97 > 79 characters)
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:83: [E501] line too long (84 > 79 characters)
1       E125 continuation line with same indent as next logical line
5       E501 line too long (80 > 79 characters)
1       W391 blank line at end of file
7
make: *** [check-pep8] Error 1

All patches need to pass that (and all other) existing tests before they can be committed. Aside from that:

- When revising patches, please send an updated one for the whole thing, rather than incremental ones. Incrementals are more work to apply and don't give us any benefit in return.

- We need to add a "do you want to continue" warning before actions like Execute or EXPLAIN are run, if there are unsaved changes in the grid.

- I think we should make the text in any cells that has been edited bold until saved, so the user can see where changes have been made (as they can with deleted rows).

- If I make two data edits and then delete a row, I get 3 entries in the History panel, all showing the same delete. I would actually argue that data edit queries that pgAdmin generates should not go into the History at all, but maybe they should be added albeit with a flag to say they're internal queries and an option to hide them. Thoughts?

- We need to think about how data editing fits in with transaction control. Right now, it seems to happen entirely outside of it - for example, I tend to work with auto commit turned off, so my connection sits idle-in-transaction following an initial select, and remains un-affected by edits. Please think about this and suggest options for us to discuss.

 

- What documentations or unit tests should I write? any guidelines here would be appreciated.

We're aiming to add unit tests to give as much coverage as possible, focussing on Jasmine, Python/API and then feature tests in that order (fast -> slow execution, which is important). So we probably want 

- one feature test to do basic end-to-end validation

- Python/API tests to exercise is_query_resultset_updatable, save_changed_data and anything else that seems relevant.

- Jasmine tests to ensure buttons are enabled/disabled as they should be, and that primary key and updatability data is tracked properly (this may not be feasible, but I'd still like it to be investigated and justified if not).

We're also a day or two away from committing a new test suite for exercising CRUD operations and the resulting reverse engineered SQL; if we can utilise that to test primary_keys.sql, that'd be good.

I am sorry but I don't understand what should be done exactly in those tests. Could you tell me where I can look at examples for feature tests, Python/API tests and Jasmine tests (preferably for features related to the query tool)?

They're all over the codebase to be honest. Some examples though:

Varions Jasmine tests: web/regression/javascript (e.g. history, slickgrid, sqleditor)

Various API tests: web/pgadmin/tools/sqleditor/tests

Feature tests: web/pgadmin/feature_tests (e.g. query_tool_*)

 

 

Once the in-place editing works, we'll need to rip out all the code related to the View/Edit data mode of the query tool. For example, there will be no need to have the Filter/Sort options any more as the user can edit the SQL directly (that one may be controversial - it's probably worth polling the users first). Of course, if they don't want it to be removed, we'll need to re-think how it works as then we'd have a dialogue that tries to edit arbitrary SQL strings.

I think it makes more sense for filters to be disabled. I mean since the user is already writing SQL it would be more convenient to just edit it directly.

Well we're not going to just disable them - we'll either remove them, or try to make them work. I'm leaning strongly towards just removing that code entirely.

Good work - thanks!

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company