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