Обсуждение: patch for cast module

Hi,

PFA patch for cast module.

Please do review it and let me know in case of any issue.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

Hi,

PFA updated patch for cast module as per check list provided by Neel.

Please do review it and let me know in case of anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Jan 18, 2016 at 7:16 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA patch for cast module.

Please do review it and let me know in case of any issue.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Hi Sanket,

Below are the review comments.

- When we edit any existing cast node then it gives error "Response object has no attribute strip". This error is coming because generated SQL is 

  wrong.

- Unnecessary debug logs are coming on console. Please remove unnecessary debug logs.

- In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. Please check all the SQL files.

- "Delete" cast functionality is not working. Error is getting displayed saying "syntax error at or near "castsource". 

- "Delete cascade" functionality is not working - error is getting displayed saying "The requested URL not found".

- Do the proper comments, in some of the function like "script_load" , comments are wrong.

- Is "configs" really required in __init__.py file ? We have not seen any usage for this. Please remove it if it is not required.

- Remove commented code from the source file.

Please check all the generated SQL statements . Test the basic functionality of "create", "Edit" and "Delete" node before sending patch file.

Do let us know for any comments/issues.

Thanks,

Neel Patel

On Tue, Jan 19, 2016 at 8:06 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA updated patch for cast module as per check list provided by Neel.

Please do review it and let me know in case of anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Jan 18, 2016 at 7:16 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA patch for cast module.

Please do review it and let me know in case of any issue.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Hi Neel.

PFA the revised patch which has changed according to your comments.

Please do review it and let me know in case anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Wed, Jan 20, 2016 at 10:20 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:

Hi Sanket,

Below are the review comments.

- When we edit any existing cast node then it gives error "Response object has no attribute strip". This error is coming because generated SQL is 

  wrong.

- Unnecessary debug logs are coming on console. Please remove unnecessary debug logs.

- In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. Please check all the SQL files.

- "Delete" cast functionality is not working. Error is getting displayed saying "syntax error at or near "castsource". 

- "Delete cascade" functionality is not working - error is getting displayed saying "The requested URL not found".

- Do the proper comments, in some of the function like "script_load" , comments are wrong.

- Is "configs" really required in __init__.py file ? We have not seen any usage for this. Please remove it if it is not required.

- Remove commented code from the source file.

Please check all the generated SQL statements . Test the basic functionality of "create", "Edit" and "Delete" node before sending patch file.

Do let us know for any comments/issues.

Thanks,

Neel Patel

On Tue, Jan 19, 2016 at 8:06 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA updated patch for cast module as per check list provided by Neel.

Please do review it and let me know in case of anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Jan 18, 2016 at 7:16 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA patch for cast module.

Please do review it and let me know in case of any issue.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Hi Akshay,

PFA the latest patch for Cast module.

Please do review it and let me know if anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Wed, Jan 20, 2016 at 5:03 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Neel.

PFA the revised patch which has changed according to your comments.

Please do review it and let me know in case anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Wed, Jan 20, 2016 at 10:20 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:

Hi Sanket,

Below are the review comments.

- When we edit any existing cast node then it gives error "Response object has no attribute strip". This error is coming because generated SQL is 

  wrong.

- Unnecessary debug logs are coming on console. Please remove unnecessary debug logs.

- In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. Please check all the SQL files.

- "Delete" cast functionality is not working. Error is getting displayed saying "syntax error at or near "castsource". 

- "Delete cascade" functionality is not working - error is getting displayed saying "The requested URL not found".

- Do the proper comments, in some of the function like "script_load" , comments are wrong.

- Is "configs" really required in __init__.py file ? We have not seen any usage for this. Please remove it if it is not required.

- Remove commented code from the source file.

Please check all the generated SQL statements . Test the basic functionality of "create", "Edit" and "Delete" node before sending patch file.

Do let us know for any comments/issues.

Thanks,

Neel Patel

On Tue, Jan 19, 2016 at 8:06 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA updated patch for cast module as per check list provided by Neel.

Please do review it and let me know in case of anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Jan 18, 2016 at 7:16 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA patch for cast module.

Please do review it and let me know in case of any issue.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

Hi Sanket 

Most of the review comments has been resolved but I found some issues with this patch 

• When select some system cast it is showing wrong query, for example when select "abstime->date" in pgAdmin3 it is showing "AS ASSIGNMENT" and with your code it is showing "AS IMPLICIT".
• For some of the system casts SQL query showing "WITH FUNCTION date" instead of "WITH FUNCTION date(abstime)" source type is not appended in query with new code.
• For casts "bit->"bit"" function and target type is not listed.
• When we create a new cast like "character->bytea" without selecting function, it creates successfully but when we select the newly created cast it shows the function "bpcharsend" in functions property. It may come for other combinations too, please verify. 
• Please fixed warnings in python file by using pep8 tool.    
  

On Mon, Feb 8, 2016 at 3:45 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Akshay,

PFA the revised patch.

All the comments are inline.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Fri, Feb 5, 2016 at 12:43 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Sanket 

Below are the review comments

• As "Show System Object" is not implemented yet, we should show all the objects by default.

Done

• As in pgAdmin3 when click on Casts (Collection) node it should show only Name, Owner and Comments. With current code it is showing all the properties.

Done.. Owner field is ignore as it is not a part of cast properties.

• Properties Tab contains only one control "Comment" can that be a part of the Definition tab???
• For some data type like "Character", "Integer", it is throwing error that data type doesn't exist.

resolved

• If node is leaf node then it should not show (+) expand symbol.

Done

• Remove extra lines from create.sql and update.sql files as it shown in the SQL tab as well.

Ignored as it was suggested by Ashesh.

• When select any system cast it is not showing function in the function control.

Resolved.

• If comment is already exist and we remove the comments, sql query not generated in the SQL tab while it is generating in pgAdmin3.

Done.
 

Question: With current implementation in "pgAdmin3" to create "Cast" user will have to select source type and target type and then click on OK button. If source and target type is not physically compatible, server will throw an error. I am not sure, but instead of that can we implement it like when user select the source type from combo box, target type combo will only show types which are physically compatible?

After consulting with db server team, it is clear that they do not maintain any mapping for compatible source and target types. in postgresql, they pick selected source and target type and check them for compatibility. So its not possible to filter out target type based on selected source type.

          

On Thu, Feb 4, 2016 at 6:31 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Akshay,

PFA the latest patch for Cast module.

Please do review it and let me know if anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Wed, Jan 20, 2016 at 5:03 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Neel.

PFA the revised patch which has changed according to your comments.

Please do review it and let me know in case anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Wed, Jan 20, 2016 at 10:20 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:

Hi Sanket,

Below are the review comments.

- When we edit any existing cast node then it gives error "Response object has no attribute strip". This error is coming because generated SQL is 

  wrong.

- Unnecessary debug logs are coming on console. Please remove unnecessary debug logs.

- In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. Please check all the SQL files.

- "Delete" cast functionality is not working. Error is getting displayed saying "syntax error at or near "castsource". 

- "Delete cascade" functionality is not working - error is getting displayed saying "The requested URL not found".

- Do the proper comments, in some of the function like "script_load" , comments are wrong.

- Is "configs" really required in __init__.py file ? We have not seen any usage for this. Please remove it if it is not required.

- Remove commented code from the source file.

Please check all the generated SQL statements . Test the basic functionality of "create", "Edit" and "Delete" node before sending patch file.

Do let us know for any comments/issues.

Thanks,

Neel Patel

On Tue, Jan 19, 2016 at 8:06 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA updated patch for cast module as per check list provided by Neel.

Please do review it and let me know in case of anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Jan 18, 2016 at 7:16 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA patch for cast module.

Please do review it and let me know in case of any issue.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--

Akshay Joshi

Principal Software Engineer 

Phone: +91 20-3058-9517
Mobile: +91 976-788-8246

--

Akshay Joshi

Principal Software Engineer 

Phone: +91 20-3058-9517
Mobile: +91 976-788-8246

On Fri, Feb 12, 2016 at 11:57 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Akshay,

PFA the updated patch.

Hi

When testing this patch, I get the following error:

Traceback (most recent call last):

  File "pgAdmin4.py", line 56, in <module>

    app = create_app()

  File "/Users/dpage/git/pgadmin4/web/pgadmin/__init__.py", line 194, in create_app

    app.register_blueprint(module)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 62, in wrapper_func

    return f(self, *args, **kwargs)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 889, in register_blueprint

    blueprint.register(self, options, first_registration)

  File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/__init__.py", line 40, in register

    app.register_blueprint(module)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 62, in wrapper_func

    return f(self, *args, **kwargs)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 889, in register_blueprint

    blueprint.register(self, options, first_registration)

  File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/__init__.py", line 40, in register

    app.register_blueprint(module)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 62, in wrapper_func

    return f(self, *args, **kwargs)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 889, in register_blueprint

    blueprint.register(self, options, first_registration)

  File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/server_groups/servers/__init__.py", line 147, in register

    super(ServerModule, self).register(app, options, first_registration)

  File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/__init__.py", line 40, in register

    app.register_blueprint(module)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 62, in wrapper_func

    return f(self, *args, **kwargs)

  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py", line 889, in register_blueprint

    blueprint.register(self, options, first_registration)

  File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/__init__.py", line 37, in register

    self.submodules = list(app.find_submodules(self.import_name))

  File "/Users/dpage/git/pgadmin4/web/pgadmin/__init__.py", line 42, in find_submodules

    module = import_module(module_name)

  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/importlib/__init__.py", line 37, in import_module

    __import__(name)

  File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/casts/__init__.py", line 21, in <module>

    from html import escape

ImportError: No module named html

Other comments:

- Leave a blank line before the header block and the first line of Python code please.

- There is no pydoc description for the module.

- The javascript is almost completely devoid of comments. There should be at least a minimal amount to explain what each code block is doing.

- There should be no blank lines at the top of SQL templates (e.g. create.sql).

--

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

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

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

Regarding your suggestion of putting some comments in javascript, I think I have already put some comments regarding model data and their controls if any extended.

Can you please let me know where exactly you think more comments are required?

Hi

The issue for me is that jQuery code isn't the easiest to read at the best of times, with nested/anonymous functions and inline JSON etc. As I look through the code for the various nodes in isolation, it's extremely difficult to get a sense of what exactly each part of the code is doing. In this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)

- Extend the collection class

- Extend the node class

  - Define an init function inline

  - Add the menu options

That part is fairly easy to figure out (easier because there are blank lines between the logical sections). From there though, it becomes much harder;

- There are no blank lines to separate logical code sections at all between line 48 and 235 (there is one blank line, but it doesn't separate code sections). 

- There are 4 comments that I can see. The first two are identical, and appear to have identical code blocks following them for reasons that are not even remotely obvious. 

- As a newcomer to this code, I'm wondering if it's purpose is to define the backform model. If so, why is it not broken up into sections with a comment to tell me what field each block handles, and any other useful information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the point is that without spending a couple of hours understanding this, I simply don't know. The point of the comments (and separation of logical sections of code with blank lines) is to make it easy for another developer (especially one as rusty as me) to read and understand, then fix and improve. Be generous with comments, but don't use them unnecessarily (e.g. "a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of us working on pgAdmin need to keep in mind.

Thanks.

--

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

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

That's much better. Just a couple of comments now, partly based on an email I wrote earlier:

- There is still inconsistency in comment style. Please see the attachment for an example. Note that there is *always* a space between the comment marker and text.

- If I try to edit a cast, I can change the description - but no SQL is shown on the SQL tab, despite the comment being correctly applied when I hit save. The properties pane of the main window is also not updated.

Otherwise, it looks fine.

Thanks.

On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA the revised patch with all the required comments.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org> wrote:

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

Regarding your suggestion of putting some comments in javascript, I think I have already put some comments regarding model data and their controls if any extended.

Can you please let me know where exactly you think more comments are required?

Hi

The issue for me is that jQuery code isn't the easiest to read at the best of times, with nested/anonymous functions and inline JSON etc. As I look through the code for the various nodes in isolation, it's extremely difficult to get a sense of what exactly each part of the code is doing. In this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)

- Extend the collection class

- Extend the node class

  - Define an init function inline

  - Add the menu options

That part is fairly easy to figure out (easier because there are blank lines between the logical sections). From there though, it becomes much harder;

- There are no blank lines to separate logical code sections at all between line 48 and 235 (there is one blank line, but it doesn't separate code sections). 

- There are 4 comments that I can see. The first two are identical, and appear to have identical code blocks following them for reasons that are not even remotely obvious. 

- As a newcomer to this code, I'm wondering if it's purpose is to define the backform model. If so, why is it not broken up into sections with a comment to tell me what field each block handles, and any other useful information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the point is that without spending a couple of hours understanding this, I simply don't know. The point of the comments (and separation of logical sections of code with blank lines) is to make it easy for another developer (especially one as rusty as me) to read and understand, then fix and improve. Be generous with comments, but don't use them unnecessarily (e.g. "a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of us working on pgAdmin need to keep in mind.

Thanks.

--

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

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

--

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

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

And this time with the attachment...

On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wrote:

That's much better. Just a couple of comments now, partly based on an email I wrote earlier:

- There is still inconsistency in comment style. Please see the attachment for an example. Note that there is *always* a space between the comment marker and text.

- If I try to edit a cast, I can change the description - but no SQL is shown on the SQL tab, despite the comment being correctly applied when I hit save. The properties pane of the main window is also not updated.

Otherwise, it looks fine.

Thanks.

On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA the revised patch with all the required comments.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org> wrote:

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

Regarding your suggestion of putting some comments in javascript, I think I have already put some comments regarding model data and their controls if any extended.

Can you please let me know where exactly you think more comments are required?

Hi

The issue for me is that jQuery code isn't the easiest to read at the best of times, with nested/anonymous functions and inline JSON etc. As I look through the code for the various nodes in isolation, it's extremely difficult to get a sense of what exactly each part of the code is doing. In this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)

- Extend the collection class

- Extend the node class

  - Define an init function inline

  - Add the menu options

That part is fairly easy to figure out (easier because there are blank lines between the logical sections). From there though, it becomes much harder;

- There are no blank lines to separate logical code sections at all between line 48 and 235 (there is one blank line, but it doesn't separate code sections). 

- There are 4 comments that I can see. The first two are identical, and appear to have identical code blocks following them for reasons that are not even remotely obvious. 

- As a newcomer to this code, I'm wondering if it's purpose is to define the backform model. If so, why is it not broken up into sections with a comment to tell me what field each block handles, and any other useful information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the point is that without spending a couple of hours understanding this, I simply don't know. The point of the comments (and separation of logical sections of code with blank lines) is to make it easy for another developer (especially one as rusty as me) to read and understand, then fix and improve. Be generous with comments, but don't use them unnecessarily (e.g. "a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of us working on pgAdmin need to keep in mind.

Thanks.

--

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

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

--

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

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

--

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

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

Hi

I've attached an update to this patch, in which I've done some word-smithing on various comments, and adjusted the SQL templates to improve the formatting.

However, it looks like it's bit-rotted, as the dependents/dependencies display is throwing Python errors. Please fix and then I think it's just about ready to commit.

Thanks.

On Fri, Feb 19, 2016 at 11:03 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

PFA the revise patch.

It includes changes according to your review comments as well as dependency/dependent part also.

Let me know in case anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 10:25 PM, Dave Page <dpage@pgadmin.org> wrote:

And this time with the attachment...

On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wrote:

That's much better. Just a couple of comments now, partly based on an email I wrote earlier:

- There is still inconsistency in comment style. Please see the attachment for an example. Note that there is *always* a space between the comment marker and text.

- If I try to edit a cast, I can change the description - but no SQL is shown on the SQL tab, despite the comment being correctly applied when I hit save. The properties pane of the main window is also not updated.

Otherwise, it looks fine.

Thanks.

On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA the revised patch with all the required comments.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org> wrote:

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

Regarding your suggestion of putting some comments in javascript, I think I have already put some comments regarding model data and their controls if any extended.

Can you please let me know where exactly you think more comments are required?

Hi

The issue for me is that jQuery code isn't the easiest to read at the best of times, with nested/anonymous functions and inline JSON etc. As I look through the code for the various nodes in isolation, it's extremely difficult to get a sense of what exactly each part of the code is doing. In this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)

- Extend the collection class

- Extend the node class

  - Define an init function inline

  - Add the menu options

That part is fairly easy to figure out (easier because there are blank lines between the logical sections). From there though, it becomes much harder;

- There are no blank lines to separate logical code sections at all between line 48 and 235 (there is one blank line, but it doesn't separate code sections). 

- There are 4 comments that I can see. The first two are identical, and appear to have identical code blocks following them for reasons that are not even remotely obvious. 

- As a newcomer to this code, I'm wondering if it's purpose is to define the backform model. If so, why is it not broken up into sections with a comment to tell me what field each block handles, and any other useful information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the point is that without spending a couple of hours understanding this, I simply don't know. The point of the comments (and separation of logical sections of code with blank lines) is to make it easy for another developer (especially one as rusty as me) to read and understand, then fix and improve. Be generous with comments, but don't use them unnecessarily (e.g. "a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of us working on pgAdmin need to keep in mind.

Thanks.

--

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

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

--

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

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

--

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

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

--

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

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

Hi,

PFA the revised patch as per your comments.
Please review it and let me know the feedback.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Tue, Feb 23, 2016 at 4:10 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

I've attached an update to this patch, in which I've done some word-smithing on various comments, and adjusted the SQL templates to improve the formatting.

However, it looks like it's bit-rotted, as the dependents/dependencies display is throwing Python errors. Please fix and then I think it's just about ready to commit.

Thanks.

On Fri, Feb 19, 2016 at 11:03 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

PFA the revise patch.

It includes changes according to your review comments as well as dependency/dependent part also.

Let me know in case anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 10:25 PM, Dave Page <dpage@pgadmin.org> wrote:

And this time with the attachment...

On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wrote:

That's much better. Just a couple of comments now, partly based on an email I wrote earlier:

- There is still inconsistency in comment style. Please see the attachment for an example. Note that there is *always* a space between the comment marker and text.

- If I try to edit a cast, I can change the description - but no SQL is shown on the SQL tab, despite the comment being correctly applied when I hit save. The properties pane of the main window is also not updated.

Otherwise, it looks fine.

Thanks.

On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA the revised patch with all the required comments.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org> wrote:

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

Regarding your suggestion of putting some comments in javascript, I think I have already put some comments regarding model data and their controls if any extended.

Can you please let me know where exactly you think more comments are required?

Hi

The issue for me is that jQuery code isn't the easiest to read at the best of times, with nested/anonymous functions and inline JSON etc. As I look through the code for the various nodes in isolation, it's extremely difficult to get a sense of what exactly each part of the code is doing. In this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)

- Extend the collection class

- Extend the node class

  - Define an init function inline

  - Add the menu options

That part is fairly easy to figure out (easier because there are blank lines between the logical sections). From there though, it becomes much harder;

- There are no blank lines to separate logical code sections at all between line 48 and 235 (there is one blank line, but it doesn't separate code sections). 

- There are 4 comments that I can see. The first two are identical, and appear to have identical code blocks following them for reasons that are not even remotely obvious. 

- As a newcomer to this code, I'm wondering if it's purpose is to define the backform model. If so, why is it not broken up into sections with a comment to tell me what field each block handles, and any other useful information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the point is that without spending a couple of hours understanding this, I simply don't know. The point of the comments (and separation of logical sections of code with blank lines) is to make it easy for another developer (especially one as rusty as me) to read and understand, then fix and improve. Be generous with comments, but don't use them unnecessarily (e.g. "a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of us working on pgAdmin need to keep in mind.

Thanks.

--

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

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

--

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

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

--

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

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

--

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

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

Thanks - committed.

On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA the revised patch as per your comments.
Please review it and let me know the feedback.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Tue, Feb 23, 2016 at 4:10 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

I've attached an update to this patch, in which I've done some word-smithing on various comments, and adjusted the SQL templates to improve the formatting.

However, it looks like it's bit-rotted, as the dependents/dependencies display is throwing Python errors. Please fix and then I think it's just about ready to commit.

Thanks.

On Fri, Feb 19, 2016 at 11:03 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

PFA the revise patch.

It includes changes according to your review comments as well as dependency/dependent part also.

Let me know in case anything is missing.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 10:25 PM, Dave Page <dpage@pgadmin.org> wrote:

And this time with the attachment...

On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wrote:

That's much better. Just a couple of comments now, partly based on an email I wrote earlier:

- There is still inconsistency in comment style. Please see the attachment for an example. Note that there is *always* a space between the comment marker and text.

- If I try to edit a cast, I can change the description - but no SQL is shown on the SQL tab, despite the comment being correctly applied when I hit save. The properties pane of the main window is also not updated.

Otherwise, it looks fine.

Thanks.

On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi,

PFA the revised patch with all the required comments.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org> wrote:

On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:

Hi Dave,

Regarding your suggestion of putting some comments in javascript, I think I have already put some comments regarding model data and their controls if any extended.

Can you please let me know where exactly you think more comments are required?

Hi

The issue for me is that jQuery code isn't the easiest to read at the best of times, with nested/anonymous functions and inline JSON etc. As I look through the code for the various nodes in isolation, it's extremely difficult to get a sense of what exactly each part of the code is doing. In this example, what I see by reading the code is:

- Define the required libraries (require.js stuff)

- Extend the collection class

- Extend the node class

  - Define an init function inline

  - Add the menu options

That part is fairly easy to figure out (easier because there are blank lines between the logical sections). From there though, it becomes much harder;

- There are no blank lines to separate logical code sections at all between line 48 and 235 (there is one blank line, but it doesn't separate code sections). 

- There are 4 comments that I can see. The first two are identical, and appear to have identical code blocks following them for reasons that are not even remotely obvious. 

- As a newcomer to this code, I'm wondering if it's purpose is to define the backform model. If so, why is it not broken up into sections with a comment to tell me what field each block handles, and any other useful information I may need to know? If it's not, then what is it for?

So... I'm not going to tell you exactly where to put comments, because the point is that without spending a couple of hours understanding this, I simply don't know. The point of the comments (and separation of logical sections of code with blank lines) is to make it easy for another developer (especially one as rusty as me) to read and understand, then fix and improve. Be generous with comments, but don't use them unnecessarily (e.g. "a = 1 // Set a to one").

Of course, this is not just directed at you Sanket - it's something all of us working on pgAdmin need to keep in mind.

Thanks.

--

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

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

--

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

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

--

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

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

--

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

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

--

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

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

Thanks, patch applied.

On Tue, Mar 1, 2016 at 7:20 AM, Sanket Mehta

<sanket.mehta@enterprisedb.com> wrote:
> Hi,
>
> There was an error in cast module while fetching its dependency and
> dependent.
> Below is the patch which resolves this issue.
> Please review and commit it.
>
>
>
> Regards,
> Sanket Mehta
> Sr Software engineer
> Enterprisedb
>
> On Wed, Feb 24, 2016 at 10:17 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Thanks - committed.
>>
>> On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta
>> <sanket.mehta@enterprisedb.com> wrote:
>>>
>>> Hi,
>>>
>>> PFA the revised patch as per your comments.
>>> Please review it and let me know the feedback.
>>>
>>> Regards,
>>> Sanket Mehta
>>> Sr Software engineer
>>> Enterprisedb
>>>
>>> On Tue, Feb 23, 2016 at 4:10 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>
>>>> Hi
>>>>
>>>> I've attached an update to this patch, in which I've done some
>>>> word-smithing on various comments, and adjusted the SQL templates to improve
>>>> the formatting.
>>>>
>>>> However, it looks like it's bit-rotted, as the dependents/dependencies
>>>> display is throwing Python errors. Please fix and then I think it's just
>>>> about ready to commit.
>>>>
>>>> Thanks.
>>>>
>>>>
>>>> On Fri, Feb 19, 2016 at 11:03 AM, Sanket Mehta
>>>> <sanket.mehta@enterprisedb.com> wrote:
>>>>>
>>>>> Hi Dave,
>>>>>
>>>>> PFA the revise patch.
>>>>>
>>>>> It includes changes according to your review comments as well as
>>>>> dependency/dependent part also.
>>>>>
>>>>> Let me know in case anything is missing.
>>>>>
>>>>> Regards,
>>>>> Sanket Mehta
>>>>> Sr Software engineer
>>>>> Enterprisedb
>>>>>
>>>>> On Mon, Feb 15, 2016 at 10:25 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>>
>>>>>> And this time with the attachment...
>>>>>>
>>>>>> On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>>>
>>>>>>> That's much better. Just a couple of comments now, partly based on an
>>>>>>> email I wrote earlier:
>>>>>>>
>>>>>>> - There is still inconsistency in comment style. Please see the
>>>>>>> attachment for an example. Note that there is *always* a space between the
>>>>>>> comment marker and text.
>>>>>>>
>>>>>>> - If I try to edit a cast, I can change the description - but no SQL
>>>>>>> is shown on the SQL tab, despite the comment being correctly applied when I
>>>>>>> hit save. The properties pane of the main window is also not updated.
>>>>>>>
>>>>>>> Otherwise, it looks fine.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta
>>>>>>> <sanket.mehta@enterprisedb.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> PFA the revised patch with all the required comments.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Sanket Mehta
>>>>>>>> Sr Software engineer
>>>>>>>> Enterprisedb
>>>>>>>>
>>>>>>>> On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta
>>>>>>>>> <sanket.mehta@enterprisedb.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Dave,
>>>>>>>>>>
>>>>>>>>>> Regarding your suggestion of putting some comments in javascript,
>>>>>>>>>> I think I have already put some comments regarding model data and their
>>>>>>>>>> controls if any extended.
>>>>>>>>>>
>>>>>>>>>> Can you please let me know where exactly you think more comments
>>>>>>>>>> are required?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> The issue for me is that jQuery code isn't the easiest to read at
>>>>>>>>> the best of times, with nested/anonymous functions and inline JSON etc. As I
>>>>>>>>> look through the code for the various nodes in isolation, it's extremely
>>>>>>>>> difficult to get a sense of what exactly each part of the code is doing. In
>>>>>>>>> this example, what I see by reading the code is:
>>>>>>>>>
>>>>>>>>> - Define the required libraries (require.js stuff)
>>>>>>>>> - Extend the collection class
>>>>>>>>> - Extend the node class
>>>>>>>>>   - Define an init function inline
>>>>>>>>>   - Add the menu options
>>>>>>>>>
>>>>>>>>> That part is fairly easy to figure out (easier because there are
>>>>>>>>> blank lines between the logical sections). From there though, it becomes
>>>>>>>>> much harder;
>>>>>>>>>
>>>>>>>>> - There are no blank lines to separate logical code sections at all
>>>>>>>>> between line 48 and 235 (there is one blank line, but it doesn't separate
>>>>>>>>> code sections).
>>>>>>>>> - There are 4 comments that I can see. The first two are identical,
>>>>>>>>> and appear to have identical code blocks following them for reasons that are
>>>>>>>>> not even remotely obvious.
>>>>>>>>> - As a newcomer to this code, I'm wondering if it's purpose is to
>>>>>>>>> define the backform model. If so, why is it not broken up into sections with
>>>>>>>>> a comment to tell me what field each block handles, and any other useful
>>>>>>>>> information I may need to know? If it's not, then what is it for?
>>>>>>>>>
>>>>>>>>> So... I'm not going to tell you exactly where to put comments,
>>>>>>>>> because the point is that without spending a couple of hours understanding
>>>>>>>>> this, I simply don't know. The point of the comments (and separation of
>>>>>>>>> logical sections of code with blank lines) is to make it easy for another
>>>>>>>>> developer (especially one as rusty as me) to read and understand, then fix
>>>>>>>>> and improve. Be generous with comments, but don't use them unnecessarily
>>>>>>>>> (e.g. "a = 1 // Set a to one").
>>>>>>>>>
>>>>>>>>> Of course, this is not just directed at you Sanket - it's something
>>>>>>>>> all of us working on pgAdmin need to keep in mind.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Dave Page
>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>
>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>
>>>
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



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

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

Hi Dave,

PFA the patch for cast module incorporating changes regarding showing system objects.

Apart from support for showing system object I have resolved few bugs in that, unnecessary code and added nodes.sql file.

Please do review it and if it looks good, please commit.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Fri, Mar 4, 2016 at 4:33 PM, Dave Page <dpage@pgadmin.org> wrote:

Thanks, patch applied.

On Tue, Mar 1, 2016 at 7:20 AM, Sanket Mehta

<sanket.mehta@enterprisedb.com> wrote:
> Hi,
>
> There was an error in cast module while fetching its dependency and
> dependent.
> Below is the patch which resolves this issue.
> Please review and commit it.
>
>
>
> Regards,
> Sanket Mehta
> Sr Software engineer
> Enterprisedb
>
> On Wed, Feb 24, 2016 at 10:17 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Thanks - committed.
>>
>> On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta
>> <sanket.mehta@enterprisedb.com> wrote:
>>>
>>> Hi,
>>>
>>> PFA the revised patch as per your comments.
>>> Please review it and let me know the feedback.
>>>
>>> Regards,
>>> Sanket Mehta
>>> Sr Software engineer
>>> Enterprisedb
>>>
>>> On Tue, Feb 23, 2016 at 4:10 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>
>>>> Hi
>>>>
>>>> I've attached an update to this patch, in which I've done some
>>>> word-smithing on various comments, and adjusted the SQL templates to improve
>>>> the formatting.
>>>>
>>>> However, it looks like it's bit-rotted, as the dependents/dependencies
>>>> display is throwing Python errors. Please fix and then I think it's just
>>>> about ready to commit.
>>>>
>>>> Thanks.
>>>>
>>>>
>>>> On Fri, Feb 19, 2016 at 11:03 AM, Sanket Mehta
>>>> <sanket.mehta@enterprisedb.com> wrote:
>>>>>
>>>>> Hi Dave,
>>>>>
>>>>> PFA the revise patch.
>>>>>
>>>>> It includes changes according to your review comments as well as
>>>>> dependency/dependent part also.
>>>>>
>>>>> Let me know in case anything is missing.
>>>>>
>>>>> Regards,
>>>>> Sanket Mehta
>>>>> Sr Software engineer
>>>>> Enterprisedb
>>>>>
>>>>> On Mon, Feb 15, 2016 at 10:25 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>>
>>>>>> And this time with the attachment...
>>>>>>
>>>>>> On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>>>>>
>>>>>>> That's much better. Just a couple of comments now, partly based on an
>>>>>>> email I wrote earlier:
>>>>>>>
>>>>>>> - There is still inconsistency in comment style. Please see the
>>>>>>> attachment for an example. Note that there is *always* a space between the
>>>>>>> comment marker and text.
>>>>>>>
>>>>>>> - If I try to edit a cast, I can change the description - but no SQL
>>>>>>> is shown on the SQL tab, despite the comment being correctly applied when I
>>>>>>> hit save. The properties pane of the main window is also not updated.
>>>>>>>
>>>>>>> Otherwise, it looks fine.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta
>>>>>>> <sanket.mehta@enterprisedb.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> PFA the revised patch with all the required comments.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Sanket Mehta
>>>>>>>> Sr Software engineer
>>>>>>>> Enterprisedb
>>>>>>>>
>>>>>>>> On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dpage@pgadmin.org>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta
>>>>>>>>> <sanket.mehta@enterprisedb.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Dave,
>>>>>>>>>>
>>>>>>>>>> Regarding your suggestion of putting some comments in javascript,
>>>>>>>>>> I think I have already put some comments regarding model data and their
>>>>>>>>>> controls if any extended.
>>>>>>>>>>
>>>>>>>>>> Can you please let me know where exactly you think more comments
>>>>>>>>>> are required?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> The issue for me is that jQuery code isn't the easiest to read at
>>>>>>>>> the best of times, with nested/anonymous functions and inline JSON etc. As I
>>>>>>>>> look through the code for the various nodes in isolation, it's extremely
>>>>>>>>> difficult to get a sense of what exactly each part of the code is doing. In
>>>>>>>>> this example, what I see by reading the code is:
>>>>>>>>>
>>>>>>>>> - Define the required libraries (require.js stuff)
>>>>>>>>> - Extend the collection class
>>>>>>>>> - Extend the node class
>>>>>>>>>   - Define an init function inline
>>>>>>>>>   - Add the menu options
>>>>>>>>>
>>>>>>>>> That part is fairly easy to figure out (easier because there are
>>>>>>>>> blank lines between the logical sections). From there though, it becomes
>>>>>>>>> much harder;
>>>>>>>>>
>>>>>>>>> - There are no blank lines to separate logical code sections at all
>>>>>>>>> between line 48 and 235 (there is one blank line, but it doesn't separate
>>>>>>>>> code sections).
>>>>>>>>> - There are 4 comments that I can see. The first two are identical,
>>>>>>>>> and appear to have identical code blocks following them for reasons that are
>>>>>>>>> not even remotely obvious.
>>>>>>>>> - As a newcomer to this code, I'm wondering if it's purpose is to
>>>>>>>>> define the backform model. If so, why is it not broken up into sections with
>>>>>>>>> a comment to tell me what field each block handles, and any other useful
>>>>>>>>> information I may need to know? If it's not, then what is it for?
>>>>>>>>>
>>>>>>>>> So... I'm not going to tell you exactly where to put comments,
>>>>>>>>> because the point is that without spending a couple of hours understanding
>>>>>>>>> this, I simply don't know. The point of the comments (and separation of
>>>>>>>>> logical sections of code with blank lines) is to make it easy for another
>>>>>>>>> developer (especially one as rusty as me) to read and understand, then fix
>>>>>>>>> and improve. Be generous with comments, but don't use them unnecessarily
>>>>>>>>> (e.g. "a = 1 // Set a to one").
>>>>>>>>>
>>>>>>>>> Of course, this is not just directed at you Sanket - it's something
>>>>>>>>> all of us working on pgAdmin need to keep in mind.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Dave Page
>>>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>
>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>
>>>
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



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

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