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(at)gmail(dot)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(at)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