I think we'll put this off for the next release (v21.2), and make this part of a wider change for formatting the entire project using automated tools. I've added a Jira issue to discuss this further: https://gem5.atlassian.net/browse/GEM5-1038.
-- Dr. Bobby R. Bruce Room 3050, Kemper Hall, UC Davis Davis, CA, 95616 web: https://www.bobbybruce.net On Thu, Jun 24, 2021 at 10:31 AM Jason Lowe-Power via gem5-dev < [email protected]> wrote: > Hi Daniel, > > Great point about accessibility and people with visual impairment. It is > important to make sure that the gem5 community is welcoming to all! > > Personally, I don't think that 79 -> 88 characters will make too much of a > difference, but there's no clear cut off. Increasing the line length is > becoming important in python as we move to adding type annotations. This > can cause function definitions to grow significantly with well-named types. > > One more data point to throw out, Google's C++ standard (which is our > baseline) says 80 characters for C++. > > PS: Going back and looking at the history of this conversation in the > community, in 2015 we bumped the line length from 78 to 79. It was 78 so > the two diff characters would still have an 80 character line. I'm not > certain why we went with 79 instead of 80... > > Cheers, > Jason > > On Thu, Jun 24, 2021 at 10:05 AM Daniel Carvalho via gem5-dev < > [email protected]> wrote: > >> Hello, >> >> My two cents: the number of columns is more important for readability and >> accessibility than for file size. The higher the number of columns, the >> more likely that users with visual impairment will have trouble finding an >> efficient-yet-comfortable setup. >> >> For example, on the one hand, my main gem5 clone's src/ has 92MB, while >> the whole dir uses 18GB - i.e., file sizes are negligible compared to their >> object counterparts, and AFAIK, changing the number of columns doesn't >> affect the size of these files. >> >> On the other hand, the laptop I'm using to type this has a tiny 14" >> screen, and is running at a high resolution (1920x1080). While using >> Monospace Regular 14 I am only able to see 166 columns (or 2x82 columns >> side by side) in gedit. Increasing the number of columns would require a >> reduction of the font size to be able to keep working with 2 windows. While >> I am physically able to do this change, some people may not. For reference, >> the max font size at which one can work (with my laptop) with a single >> window would have to go down from 29 to 25. >> >> As I said, this is my tiny on-the-go setup. Others may have better >> setups, and those with visual impairment will likely invest more to have >> larger screens, or use different resolutions, so it may not affect them >> that much. Yet, I think this is a much stronger point to be considered than >> file size. >> >> Regarding having more columns, how readable are long lines? Are they good >> coding practice, or would it be better to split them into multiple >> single-line statements? >> >> Finally, I'd also rather have consistency; if this number changes, it'd >> be better to do so for all file extensions, not only Python files. >> >> That said, I'm against this change as is due to the discrepancy between >> advantages and disadvantages. >> >> Regards, >> Daniel >> Em quarta-feira, 23 de junho de 2021 18:00:22 BRT, Bobby Bruce via >> gem5-dev <[email protected]> escreveu: >> >> >> I attempted to take some initiative with this relation chain: >> https://gem5-review.googlesource.com/c/public/gem5/+/47000/ >> >> In short, I applied Python Black with a line length of 79, then added a >> Python black check to our pre-submit (Kokoro) tests. >> >> This patchset raised a question as to what line length we really want for >> Python. Our official policy has been <=79 characters but Black defaults to >> 88 (The black developers argue this default produces a good trade off >> between file length and line length). This is a good chance to have this >> discussion, so the floor is open. Personally, I'm not massively opposed but >> a consistent 79 character limit across the project has worked for us thus >> far and I don't personally see much reason to change. >> >> Kind regards, >> Bobby >> -- >> Dr. Bobby R. Bruce >> Room 3050, >> Kemper Hall, UC Davis >> Davis, >> CA, 95616 >> >> web: https://www.bobbybruce.net >> >> >> On Thu, Mar 4, 2021 at 8:05 AM Giacomo Travaglini via gem5-dev < >> [email protected]> wrote: >> >> >> >> > -----Original Message----- >> > From: Jason Lowe-Power <[email protected]> >> > Sent: 04 March 2021 15:27 >> > To: gem5 Developer List <[email protected]> >> > Cc: Gabe Black <[email protected]>; Giacomo Travaglini >> > <[email protected]> >> > Subject: Re: [gem5-dev] Re: RFC: run python Black on gem5 python code >> > >> > Hi all, >> > >> > >> > Some responses inline below. The other thing I'll mention is that I >> believe we >> > can integrate it in a way where it's completely invisible to most >> developers. >> > You can edit a file with whatever style you want, then after you run >> black it will >> > be in the PEP8 style. From my experience, the only things that need to >> be >> > manually changed are quoted strings that are more than 80 characters. >> > >> >> I would be very careful about hiding this to the developers at the point >> of automatically changing their code >> without having them noticing it. >> >> I understand you want developers to commit without the hassle of >> explicitly dealing with style errors. >> On the other hand though we are already doing this for C++ so I don't see >> a reason why we shouldn't do the same >> for the python world. >> >> Other options could be to run the linter as a pre-push hook, or to run >> the linter as part of our presubmit Jenkins script. >> >> > On Thu, Mar 4, 2021 at 6:49 AM Giacomo Travaglini via gem5-dev <gem5- >> > [email protected] <mailto:[email protected]> > wrote: >> > >> > >> > Hi Jason, >> > >> > This is great news. When you say it is not configurable, do you mean it >> > is not possible to suppress warning/errors or do you >> > mean it is not possible to come up with custom rules? >> > >> > >> > >> > There is a way to suppress changes by modifying the file with a comment >> `# >> > fmt: off`. You can also exclude files with a dotfile (like .gitignore). >> > >> > >> > >> > As I presume it's the first case, I am wondering if a different linter >> > might provide such capabilities: >> > >> > https://books.agiliq.com/projects/essential-python- >> > tools/en/latest/linters.html >> > >> > Is there a reason to prefer black over other linters? >> > >> > >> > >> > It's owned by the python software foundation, which makes it seem >> "official". >> > Though the real reason is that it's the first one I looked at and the >> only one I >> > investigated. >> >> I actually thought pycodestyle was the "official" linter (I am not sure >> if the "official" word makes completely sense though) >> >> I was asking this because for example in pycodestyle (and prob other >> linters as well) there are error codes >> https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes you could >> use include or exclude lists of errors (to ignore specific types of >> warnings). >> >> Kind regards >> >> Giacomo >> >> > >> > >> > >> > Kind regards >> > >> > Giacomo >> > >> > > -----Original Message----- >> > > From: Gabe Black via gem5-dev <[email protected] >> > <mailto:[email protected]> > >> > > Sent: 04 March 2021 03:59 >> > > To: gem5 Developer List <[email protected] <mailto:gem5- >> > [email protected]> > >> > > Cc: Gabe Black <[email protected] >> > <mailto:[email protected]> > >> > > Subject: [gem5-dev] Re: RFC: run python Black on gem5 python code >> > > >> > > I'm a little worried about the no exceptions part of that, since we >> > might have >> > > some weird restrictions that we have to do weird things to work >> > around, but I >> > > can't really think of an example of that off hand. I'd want to look >> at it >> > to see >> > > how much wiggle room there is in the style, since I think ironclad >> > rules which >> > > make no accommodation for occasional common sense are maybe >> > more >> > > trouble than they're worth. I'm not opposed to having at least some >> > stated >> > > standard for python though, and the "official" one seems like a >> > pretty >> > > reasonable choice. I guess it's fine with me, up until it causes me >> > some sort of >> > > problem :-) >> > >> > >> > >> > I actually think it's a feature not a bug. With an "uncompromising >> Python code >> > formatter" there can be no arguments in code review about the style. >> > >> > >> > > >> > > Maybe the right thing to do would be to give it a shot but not make >> > it >> > > compulsory until we have a feeling for how much trouble it is. >> > > >> > > >> > > Gabe >> > > >> > > On Wed, Mar 3, 2021 at 11:24 AM Bobby Bruce via gem5-dev <gem5- >> > > [email protected] <mailto:[email protected]> <mailto:gem5- >> > [email protected] <mailto:[email protected]> > > wrote: >> > > >> > > >> > > Sounds like a good idea to me. >> > > --- >> > > >> > > Dr. Bobby R. Bruce >> > > Room 2235, >> > > Kemper Hall, UC Davis >> > > Davis, >> > > CA, 95616 >> > > >> > > >> > > web: https://www.bobbybruce.net >> > > >> > > >> > > >> > > On Wed, Mar 3, 2021 at 10:11 AM Daniel Carvalho via gem5-dev >> > > <[email protected] <mailto:[email protected]> >> > <mailto:[email protected] <mailto:[email protected]> > > wrote: >> > > >> > > >> > > +1 >> > > >> > > >> > > Em quarta-feira, 3 de março de 2021 14:35:57 BRT, Jason >> > > Lowe-Power via gem5-dev <[email protected] <mailto:gem5- >> > [email protected]> <mailto:gem5- <mailto:gem5-> >> > > [email protected] <mailto:[email protected]> > > escreveu: >> > > >> > > >> > > Hi all, >> > > >> > > Right now, we don't have an official style guide for python. >> > > Our style guide >> > > >> > (http://www.gem5.org/documentation/general_docs/development/coding_st >> > > yle/) is very C++ focused. >> > > >> > > I would like to propose adopting a relatively strict PEP 8 style >> > > guide: https://www.python.org/dev/peps/pep-0008. This is the >> > "official" style >> > > guide for python (as much as there is anything official). I say >> > "relatively strict" >> > > to mean that we will limit our exceptions *as much as possible*. >> > > >> > > To implement this, Andreas S. recently pointed me to the >> > > "Black" package (https://pypi.org/project/black/) which automatically >> > fixes >> > > code style. I just tried it out with gem5art (patch coming soon) and >> > found that >> > > it works really well. The only downside is that it's not configurable >> at >> > all. >> > > Adding special cases would be almost impossible. >> > > >> > > Concrete and specific proposal: >> > > - Adopt PEP 8 as our official style guide >> > > - Use black to reformat all python code in src/ >> > > - Use black to reformat code in configs/ >> > > - Use black to reformat other python code >> > > >> > > - Use black as part of our commit hook to make sure all future >> > > python is formatted to PEP 8 >> > > >> > > Let me know what you think! >> > > >> > > Cheers, >> > > Jason >> > > >> > > _______________________________________________ >> > > gem5-dev mailing list -- [email protected] <mailto:gem5- >> > [email protected]> <mailto:gem5- <mailto:gem5-> >> > > [email protected] <mailto:[email protected]> > >> > > To unsubscribe send an email to [email protected] >> > <mailto:[email protected]> >> > > <mailto:[email protected] <mailto:gem5-dev- >> > [email protected]> > >> > > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> > > _______________________________________________ >> > > gem5-dev mailing list -- [email protected] <mailto:gem5- >> > [email protected]> <mailto:gem5- <mailto:gem5-> >> > > [email protected] <mailto:[email protected]> > >> > > To unsubscribe send an email to [email protected] >> > <mailto:[email protected]> >> > > <mailto:[email protected] <mailto:gem5-dev- >> > [email protected]> > >> > > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> > > >> > > _______________________________________________ >> > > gem5-dev mailing list -- [email protected] <mailto:gem5- >> > [email protected]> <mailto:gem5- <mailto:gem5-> >> > > [email protected] <mailto:[email protected]> > >> > > To unsubscribe send an email to [email protected] >> > <mailto:[email protected]> >> > > <mailto:[email protected] <mailto:gem5-dev- >> > [email protected]> > >> > > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> > >> > IMPORTANT NOTICE: The contents of this email and any attachments >> > are confidential and may also be privileged. If you are not the intended >> > recipient, please notify the sender immediately and do not disclose the >> > contents to any other person, use it for any purpose, or store or copy >> the >> > information in any medium. Thank you. >> > _______________________________________________ >> > gem5-dev mailing list -- [email protected] <mailto:gem5- >> > [email protected]> >> > To unsubscribe send an email to [email protected] >> > <mailto:[email protected]> >> > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> >> IMPORTANT NOTICE: The contents of this email and any attachments are >> confidential and may also be privileged. If you are not the intended >> recipient, please notify the sender immediately and do not disclose the >> contents to any other person, use it for any purpose, or store or copy the >> information in any medium. Thank you. >> _______________________________________________ >> gem5-dev mailing list -- [email protected] >> To unsubscribe send an email to [email protected] >> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> >> _______________________________________________ >> gem5-dev mailing list -- [email protected] >> To unsubscribe send an email to [email protected] >> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> _______________________________________________ >> gem5-dev mailing list -- [email protected] >> To unsubscribe send an email to [email protected] >> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s > > _______________________________________________ > gem5-dev mailing list -- [email protected] > To unsubscribe send an email to [email protected] > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
_______________________________________________ gem5-dev mailing list -- [email protected] To unsubscribe send an email to [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
