You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Verdan Mahmood <ve...@gmail.com> on 2020/12/17 13:45:39 UTC
Review Request 73097: ATLAS-4086: Python Client Fixes - Basic Search
and Bulk Entities using GUIDs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/
-----------------------------------------------------------
Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
Bugs: ATLAS-4086
https://issues.apache.org/jira/browse/ATLAS-4086
Repository: atlas
Description
-------
Python Client Fixes
Diffs
-----
intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
Diff: https://reviews.apache.org/r/73097/diff/1/
Testing
-------
- This fixes the indentation according to pep8.
- Fixes the basic search using post method, it was broken.
- Getting Bulk entities using GUIDs was broken, it also fix that.
Thanks,
Verdan Mahmood
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by nokvrlmsiyqrwuAB nokvrlmsyquaprAB <lt...@yandex.com>.
> On Дек. 17, 2020, 8:27 п.п., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/client/discovery.py
> > Line 25 (original), 28 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243712#file2243712line28>
> >
> > While I appreciate many guidelines in pep8, I find having assignment operators vertically aligned significantly improves code readability - especially for reviewers. I strongly suggest to revert all such changes in this patch.
yes
- nokvrlmsiyqrwuAB
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
On Дек. 17, 2020, 1:45 п.п., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Дек. 17, 2020, 1:45 п.п.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by Verdan Mahmood <ve...@gmail.com>.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/client/discovery.py
> > Line 25 (original), 28 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243712#file2243712line28>
> >
> > While I appreciate many guidelines in pep8, I find having assignment operators vertically aligned significantly improves code readability - especially for reviewers. I strongly suggest to revert all such changes in this patch.
>
> nokvrlmsiyqrwuAB nokvrlmsyquaprAB wrote:
> yes
I have a completely different opinion on this, as this is not pythonic in any way. While I understand, this may be helpful for the reviews for many of the JVM engineers, but this is completely wrong from Python's perspective, and IMHO we should respect the language's syntax.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/model/instance.py
> > Line 70 (original), 74 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243714#file2243714line74>
> >
> > Many coding standards suggest line length of 80, but often adhering to this will result in poor readability - like in this case.
> >
> > I believe these guidelines were applicable for long gone era when folks printed code on paper, and/or the monitors (green-text?) were too narrow to render long lines.
> >
> > Wide monitors, powerful IDEs and lack of the need to print code on paper should enable removing this unnecessary line length limit, and significantly improve code readability.
> >
> > I strongly suggest to revert all such changes.
I agree on this. as the only reason to specify the line length is to enforce the consistency. And if we agree on one line length, I am perfectly fine with this. BUT in any case, we should specify the maximum line length. For many new projects, we are using `120` as the max line length.
- Verdan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
On Dec. 17, 2020, 1:45 p.m., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2020, 1:45 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by Mariusz Górski <go...@gmail.com>.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/model/instance.py
> > Line 70 (original), 74 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243714#file2243714line74>
> >
> > Many coding standards suggest line length of 80, but often adhering to this will result in poor readability - like in this case.
> >
> > I believe these guidelines were applicable for long gone era when folks printed code on paper, and/or the monitors (green-text?) were too narrow to render long lines.
> >
> > Wide monitors, powerful IDEs and lack of the need to print code on paper should enable removing this unnecessary line length limit, and significantly improve code readability.
> >
> > I strongly suggest to revert all such changes.
>
> Verdan Mahmood wrote:
> I agree on this. as the only reason to specify the line length is to enforce the consistency. And if we agree on one line length, I am perfectly fine with this. BUT in any case, we should specify the maximum line length. For many new projects, we are using `120` as the max line length.
>
> Madhan Neethiraj wrote:
> 120 is much better than 80. However, the key is readablity than complying to a number.
I think another angle in this discussion would be to look at Python client from community and developers perspective.
PEP is a de-facto standard for Python formatting and if we want encourage Pythonistas to develop/extend it then we should consider how they work. While developing Python code in JVM-native IDE (like Eclipse or IntelliJ) won't raise any flags, these minor things will be a pain points for any IDE designed to work with Python (like Pycharm) - any kind of deviation from standard looks messy while developing. And if Pythonista is reviewing the code, then he/she will have much easier job looking at a familiar syntax. Why not make this part of Atlas familiar to them ? If full set of best practices/tools around Python development (like linting, static type validation) was to be enabled (and imo it should) there is no way around it really.
I don't mean to start off yet another argument like 'spaces vs tabs' but having experience with different Open Source projects enabling clients for multiple languages I always appreciated when every client is designed and aligned with standards applicable to it and familiar to native users. In the end, every contribution should be for a good of end users, which might come from differnt backgrounds.
Having said that, I think fixing code anf reformatting it should be two separate PRs so one doesn't block another.
- Mariusz
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
On Dec. 17, 2020, 1:45 p.m., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2020, 1:45 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/model/instance.py
> > Line 31 (original), 31 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243714#file2243714line31>
> >
> > What is the advantage of replacing argument default value from {} to None, and handling None here? Also, with this change, the call above to AtlasBase.__init__() would send None.
> >
> > Unless there is a compelling reason, I suggest to retain existing default value of {}.
The change you suggested, to use None as default value, is indeed correct - based on Python default value instantiation. I take back my comment. Thanks!
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
On Dec. 17, 2020, 1:45 p.m., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2020, 1:45 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/client/discovery.py
> > Line 25 (original), 28 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243712#file2243712line28>
> >
> > While I appreciate many guidelines in pep8, I find having assignment operators vertically aligned significantly improves code readability - especially for reviewers. I strongly suggest to revert all such changes in this patch.
>
> nokvrlmsiyqrwuAB nokvrlmsyquaprAB wrote:
> yes
>
> Verdan Mahmood wrote:
> I have a completely different opinion on this, as this is not pythonic in any way. While I understand, this may be helpful for the reviews for many of the JVM engineers, but this is completely wrong from Python's perspective, and IMHO we should respect the language's syntax.
Isn't this a chosen coding-style than language syntax? It is difficult to arrive at consensus on many such coding-styles, hence I suggest to not update existing code.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/model/instance.py
> > Line 70 (original), 74 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243714#file2243714line74>
> >
> > Many coding standards suggest line length of 80, but often adhering to this will result in poor readability - like in this case.
> >
> > I believe these guidelines were applicable for long gone era when folks printed code on paper, and/or the monitors (green-text?) were too narrow to render long lines.
> >
> > Wide monitors, powerful IDEs and lack of the need to print code on paper should enable removing this unnecessary line length limit, and significantly improve code readability.
> >
> > I strongly suggest to revert all such changes.
>
> Verdan Mahmood wrote:
> I agree on this. as the only reason to specify the line length is to enforce the consistency. And if we agree on one line length, I am perfectly fine with this. BUT in any case, we should specify the maximum line length. For many new projects, we are using `120` as the max line length.
120 is much better than 80. However, the key is readablity than complying to a number.
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
On Dec. 17, 2020, 1:45 p.m., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2020, 1:45 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by Madhan Neethiraj <ma...@apache.org>.
> On Dec. 17, 2020, 8:27 p.m., Madhan Neethiraj wrote:
> > intg/src/main/python/apache_atlas/model/instance.py
> > Line 70 (original), 74 (patched)
> > <https://reviews.apache.org/r/73097/diff/1/?file=2243714#file2243714line74>
> >
> > Many coding standards suggest line length of 80, but often adhering to this will result in poor readability - like in this case.
> >
> > I believe these guidelines were applicable for long gone era when folks printed code on paper, and/or the monitors (green-text?) were too narrow to render long lines.
> >
> > Wide monitors, powerful IDEs and lack of the need to print code on paper should enable removing this unnecessary line length limit, and significantly improve code readability.
> >
> > I strongly suggest to revert all such changes.
>
> Verdan Mahmood wrote:
> I agree on this. as the only reason to specify the line length is to enforce the consistency. And if we agree on one line length, I am perfectly fine with this. BUT in any case, we should specify the maximum line length. For many new projects, we are using `120` as the max line length.
>
> Madhan Neethiraj wrote:
> 120 is much better than 80. However, the key is readablity than complying to a number.
>
> Mariusz Górski wrote:
> I think another angle in this discussion would be to look at Python client from community and developers perspective.
>
> PEP is a de-facto standard for Python formatting and if we want encourage Pythonistas to develop/extend it then we should consider how they work. While developing Python code in JVM-native IDE (like Eclipse or IntelliJ) won't raise any flags, these minor things will be a pain points for any IDE designed to work with Python (like Pycharm) - any kind of deviation from standard looks messy while developing. And if Pythonista is reviewing the code, then he/she will have much easier job looking at a familiar syntax. Why not make this part of Atlas familiar to them ? If full set of best practices/tools around Python development (like linting, static type validation) was to be enabled (and imo it should) there is no way around it really.
>
> I don't mean to start off yet another argument like 'spaces vs tabs' but having experience with different Open Source projects enabling clients for multiple languages I always appreciated when every client is designed and aligned with standards applicable to it and familiar to native users. In the end, every contribution should be for a good of end users, which might come from differnt backgrounds.
>
> Having said that, I think fixing code anf reformatting it should be two separate PRs so one doesn't block another.
Thanks for adding more context. If sticking to PEP8 guidelines makes it easier for users of major Python IDEs, we can go with the guidelines.
+1 to handle reformatting in a separate patch.
@Verdan - can you please update this patch, to only include the fixes?
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
On Dec. 17, 2020, 1:45 p.m., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2020, 1:45 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>
Re: Review Request 73097: ATLAS-4086: Python Client Fixes - Basic
Search and Bulk Entities using GUIDs
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73097/#review222360
-----------------------------------------------------------
intg/src/main/python/apache_atlas/client/discovery.py
Line 25 (original), 28 (patched)
<https://reviews.apache.org/r/73097/#comment311418>
While I appreciate many guidelines in pep8, I find having assignment operators vertically aligned significantly improves code readability - especially for reviewers. I strongly suggest to revert all such changes in this patch.
intg/src/main/python/apache_atlas/model/instance.py
Line 31 (original), 31 (patched)
<https://reviews.apache.org/r/73097/#comment311416>
What is the advantage of replacing argument default value from {} to None, and handling None here? Also, with this change, the call above to AtlasBase.__init__() would send None.
Unless there is a compelling reason, I suggest to retain existing default value of {}.
intg/src/main/python/apache_atlas/model/instance.py
Line 70 (original), 74 (patched)
<https://reviews.apache.org/r/73097/#comment311417>
Many coding standards suggest line length of 80, but often adhering to this will result in poor readability - like in this case.
I believe these guidelines were applicable for long gone era when folks printed code on paper, and/or the monitors (green-text?) were too narrow to render long lines.
Wide monitors, powerful IDEs and lack of the need to print code on paper should enable removing this unnecessary line length limit, and significantly improve code readability.
I strongly suggest to revert all such changes.
- Madhan Neethiraj
On Dec. 17, 2020, 1:45 p.m., Verdan Mahmood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73097/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2020, 1:45 p.m.)
>
>
> Review request for atlas, Madhan Neethiraj, Pinal Shah, and Sarath Subramanian.
>
>
> Bugs: ATLAS-4086
> https://issues.apache.org/jira/browse/ATLAS-4086
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Python Client Fixes
>
>
> Diffs
> -----
>
> intg/src/main/python/apache_atlas/client/discovery.py 5bead22e776c00988e7b19b0e6e81e8a3d3175df
> intg/src/main/python/apache_atlas/model/discovery.py 61a992ce7a86f3f4c75792cfe66d87dac8ca991c
> intg/src/main/python/apache_atlas/model/instance.py 89dbe350587d66442e599598908179c6ac8334d5
>
>
> Diff: https://reviews.apache.org/r/73097/diff/1/
>
>
> Testing
> -------
>
> - This fixes the indentation according to pep8.
> - Fixes the basic search using post method, it was broken.
> - Getting Bulk entities using GUIDs was broken, it also fix that.
>
>
> Thanks,
>
> Verdan Mahmood
>
>