Re: [GSoC] Finalized First Patch

Поиск
Список
Период
Сортировка
От Khushboo Vashi
Тема Re: [GSoC] Finalized First Patch
Дата
Msg-id CAFOhELdJaTNb7x6GEAMw7yZ-R=J76xQk1AjdhGVArWLi7sM7vg@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [GSoC] Finalized First Patch  (Dave Page <dpage@pgadmin.org>)
Список pgadmin-hackers


On Fri, Jul 5, 2019 at 4:31 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <yosrym93@gmail.com> wrote:
- The patch won't apply with "git apply" and only partially applies with patch -p0. Please "git add" all your changes and new files in your repo, and then run "git diff --cached --binary", which should create a usable patch. You can then un-stage your changes.


That was due to new image. I made an applicable one with the below modifications, please find it attached.

Thanks!
 
 
- execute_query_utils.py is somewhat lacking in file header and any comments.


Added.

- There is commented-out code in sqleditor.js

This is the call that adds queries that are generated by pgAdmin to the query history. I commented it instead of removing it as I will add it later with some modifications when I add the checkbox.

Sure, but we won't commit commented-out code. It just makes things messy until such time as it gets used (or more often, does not).
 
- On reflection, I don't think the "Data saved successfully, you still need to commit changes to the database." is prominent enough - in testing, I found it very easy to miss. That might also be compounded by the fact that "Alert on uncommitted transactions?" doesn't seem to be working for me. I get the "Save text" prompt to save the query text, but if I say no to that, the Query Tool instance closes with no further warning, despite having a transaction in progress.

I made a separate notification for the uncommitted save to make it more visible, check it out.

That's definitely clearer now. It avoids the "blindness" caused by the fact that you always get the green message.
 
I tried the scenario you provided and the uncommitted alert worked fine, could you please try again and tell me the exact scenario where that happened?

Hmm, it's working fine for me today too. Definitely wasn't yesterday though!

I'm going to make some minor tweaks to the wording of the docs before I commit (as well as removing the commented-out code), but I think this is good to go, once it's had another review. Khushboo, please take a look as soon as you can.

Sure. I am on it. 
Thanks! 

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

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

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

Предыдущее
От: Dave Page
Дата:
Сообщение: pgAdmin 4 commit: Add an example of reverse proxying from HTTPS to HTTP
Следующее
От: Nagesh Dhope
Дата:
Сообщение: Re: RE-SQL test patch for extension node