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

Поиск
Список
Период
Сортировка
От Dave Page
Тема Re: [GSoC][Patch] Automatic Mode Detection V1
Дата
Msg-id CA+OCxozpBQUfmBMuZ8EXN2NRM4Y5pyCmY3SOq6AnbC5VeLG70A@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [GSoC][Patch] Automatic Mode Detection V1  (Yosry Muhammad <yosrym93@gmail.com>)
Ответы Re: [GSoC][Patch] Automatic Mode Detection V1  (Aditya Toshniwal <aditya.toshniwal@enterprisedb.com>)
Список pgadmin-hackers
Hi

On Wed, Jun 19, 2019 at 6:18 AM Yosry Muhammad <yosrym93@gmail.com> wrote:

Waiting for the icon, will set it up once it is ready.

It's underway :-)
 
I ran pep8 checks and JS tests on this patch, however I could not run python tests due to a problem with chromedriver (working on it), please let me know if any tests fail.

Take a look in the Makefile (or web/regression/README) and you'll see how you can run tests selectively - e.g. to avoid the feature tests when running the Python suite, you can do "python regression/runtests.py --exclude feature_tests"

As for chromedriver, there's a utility (tools/get_chromedriver.py) you can use to download and install the correct version. You should save it to somewhere in your path; I'd suggest the bin/ directory in your virtual environment.
 

- 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.


The attached patch is a single patch including all old and new increments.

:-)

Aditya; can you do a quick code review please? Bear in mind it's a work in progress and there are no docs or tests etc. yet.
 

- 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).

Both done, new rows are highlighted too.

Nice! I realise it's most likely not your code, but if you can fix the wrapping so it doesn't break mid-word, that would be good. See the attached screenshot to see what I mean.
 

- 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?

That was a bug with the existing 'save changes' action of 'View Data', on which mine is based upon. I fixed the bug, now the queries are shown correctly. However, the queries are shown in the form in which they are sent from the backend to the database driver (without parameters - also an already existing bug in View Data Mode), for example:

INSERT INTO public.kweek (
media_url, username, text, created_at) VALUES (
%(media_url)s::character varying, %(username)s::character varying, %(text)s::text, %(created_at)s::timestamp without time zone)
 returning id;
 
I propose two solutions:
1- Hide pgadmin's generated sql from history (in both modes).
2- Show the actual sql query that was executed after the parameters are plugged in (more understandable and potentially helpful).

I like the idea of doing 2 - but I think we should have a checkbox on the history panel to show/hide generated queries (and we should include transaction control - BEGIN, COMMIT etc - in the generated query class).
 


- 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.

I integrated the data editing in the transaction control as you noted. Now the behavior is as follows:
1- In View Data mode, same existing behavior.
2- In Query Tool mode:
- If auto-commit is on: the modifications are made and commited once save is pressed.
- If auto-commit is off: the modifications are made as part of the ongoing transaction (or a new one if no transaction is ongoing), they are not commited unless the user executes a commit command (or rollback).

That seems to work. I think we need to make it more obvious that there's a transaction in progress - especially as that can be the case after the user hits the Save button and thinks their data is safe (a side-thought is that perhaps we shouldn't require the Save button to be pressed when auto-commit is turned off, as that's just odd). We should highlight the transaction state more clearly to the user, and make sure we prompt for confirmation if they try to close the tab or the whole window.
 
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.

 
I meant disabling them in the query tool while keeping them in the View Data mode as the user cannot edit the sql in the View Data mode. Do you want to remove the feature from both modes completely?

I think you misunderstand - I want to remove the View Data mode entirely. Your work should replace it.

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Вложения

В списке pgadmin-hackers по дате отправления:

Предыдущее
От: Akshay Joshi
Дата:
Сообщение: pgAdmin 4 commit: Capitalize the word 'export' used in Import/Export mo
Следующее
От: Yosry Muhammad
Дата:
Сообщение: Re: [GSoC][Patch] Automatic Mode Detection V1