Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from 88 character lines back to 80 #177

Closed
prisae opened this issue Mar 23, 2019 · 14 comments
Closed

Switch from 88 character lines back to 80 #177

prisae opened this issue Mar 23, 2019 · 14 comments
Labels
question Further information is requested

Comments

@prisae
Copy link
Member

prisae commented Mar 23, 2019

As much as I like black, and as much as I understand its advantages, its default maximum line length drives me crazy. The maximum of 80 is there for a good reason, as terminals usually have this limit. So if edit in a traditional editor, or look at the docs or the code in a terminal, a longer line-length makes working very difficult. And this is not that uncommon, think, e.g., about working on a server without GUI. I guess this is a plea that not everyone is working in a Notebook or with Atom, Sublime, VSCode etc.

I suggest to restrict the max-line-length to 80. As far as I could see, there are three places currently with a max-line-length:

verde-master/> grep -rin 'line-length'
    .pylintrc:109: max-line-length=100
    .stickler.yml:6: max-line-length: 88
    setup.cfg:10: max-line-length = 88

I attach here two screenshots, one from GVim and one from an IPython console.

IPython: Desktop-verde-master_002
coordinates py (~-Desktop-verde-master-verde) - GVIM_001

The examples are for documentation, but the same applies for the code.

@welcome
Copy link

welcome bot commented Mar 23, 2019

👋 Thanks for opening your first issue here! Please make sure you filled out the template with as much detail as possible.

You might also want to take a look at our Contributing Guide and Code of Conduct.

@djhoese
Copy link
Contributor

djhoese commented Mar 23, 2019

I am not a verde developer, just a follower of the project. In my opinion, 80 is way too limiting. No modern terminal is limited to 80 characters. On my own macbook with my default/preferred font size I am able to have a terminal window take up half my 15" screen with 100 characters:

image

For all of my projects I use 120 characters, but try to keep docstrings to 80-100. I personally don't think 100 is too much to ask for.

@ahartikainen
Copy link

On ArviZ / PyStan3 we use -l 100.

@prisae
Copy link
Member Author

prisae commented Mar 23, 2019

Fair point @djhoese, but in my opinion, very excluding. You assume that everyone has big screens with high resolution. Although that becomes more the standard, it still isn't reality for everyone in the world. Having a terminal and an editor side by side with a bigger width than 80 is still not possible in many cases.

More importantly, and as far as I know, 80 is the standard in every terminal and at least some editors. So by using the defaults setting of black you force your users to adjust their terminal. Just for your package. I don't buy that.

But again, that is my personal opinion. I thought I mention it, but feel free to simply close it @leouieda, if you don't agree.

(I wanted to open an issue on the black-GitHub page. But they have already many heated discussion about it there. I think it is good that everyone has the freedom to choose whatever they want. But having the default at 88 is a crime, in my opinion. Someone might use the defaults not even realizing she/he is severely limiting the experience for a part of its user base, and break longstanding standards.)

@prisae
Copy link
Member Author

prisae commented Mar 23, 2019

(I also forgot to mentioned visually impaired [having myself not the best sight] and elderly folks, who will thank you very much if they have not to set a higher resolution to accommodate more characters per line.)

@djhoese
Copy link
Contributor

djhoese commented Mar 23, 2019

Fair point @djhoese, but in my opinion, very excluding.

Yes, you are right, sorry. I was mostly addressing your statement that most terminals default to 80. That has not been my experience, but maybe that's because I usually immediately grab the corner and make the window bigger.

I also agree that in some cases like docstrings, 80 characters are easier to read. It has been my experience that in some coding situations having a hard limit of 80 can make the code uglier and harder to read. In general these are special cases though.

@prisae
Copy link
Member Author

prisae commented Mar 23, 2019

in some coding situations having a hard limit of 80 can make the code uglier and harder to read

I agree. Specifically, if you follow the suggestions of having very_verbose_parameter_names in python, together with the 4 space indentation.

However, in many cases it also makes code more readable if long statements are broken-up into more lines.

@leouieda
Copy link
Member

@prisae thanks for sharing your experience 👍 I personally never have a terminal that's not full screen so the 88 limit was always fine to me. But you have a good point, particularly regarding the docstrings.

I don't particularly mind switching since it's automatic and doesn't really require much work. I'll give it a try to see if how big of a hassle it will be. I'll keep flake8 and pylint set to 88, though.

All in all, I'd rather have you to hack away at the code happily than adhere to a standard. Well, this particular standard :)

@leouieda
Copy link
Member

OK, the code changes aren't that bad and I'd be happy to switch. The problem is that black still doesn't format docstrings or comments. So all of the docstrings and comments will have to manually wrapped at 80 char. It's not too hard in Vim but still requires opening all of the files one by one because the RST identation is not always respected by vim. This is something I'm looking forward to doing right now. It's not just the code, there is also all of the examples and tutorials.

I'll leave this open for now in case someone wants to take a stab at it. Or for a time when I'm tired and need to do something mindless :)

@leouieda leouieda changed the title Set max-line-length to 80 Switch from 88 character lines back to 80 Mar 24, 2019
@leouieda
Copy link
Member

Another concern is: If we do this now, will someone come up later and complain that we need to switch to 100 char? I really don't like going back and forth and chose black specifically so we wouldn't have to think about it.

@prisae is there any way that we could make this better for you without having to touch the whole code base?

@leouieda leouieda added question Further information is requested and removed help wanted labels Mar 24, 2019
@prisae
Copy link
Member Author

prisae commented Mar 24, 2019

I read more yesterday on the black issues, and it seems to be a very heated and opinionated discussion, ranging from 80 to 100, 120, and more. I did not intended to start a flame war. I just looked up the docs of a functionality of verde yesterday an noted the formatting. I thought I report it, as I am sure that many who use black are not even aware that it sets the default limit to 88, and what the implications are. 80 was the standard for decades, since the IBM punchcards. And even though modern monitors have screen-widths and resolutions which easily permit longer line lengths, I personally think there are good reasons and lots of legacy stuff that make me prefer 80 (some of which I stated above). But then again, maybe, I am just too old school 😉 .

I agree in that it is primarily important for docstrings. If the odd line of code is longer than 80 it doesn't disturb the experience that much.

So I close this issue, and I apologize for the tempest in a tea pot. If you want to take away anything from this discussion @leouieda, then I suggest to limit docstrings to 80 characters. But from now on, hence every new code or if you change something in existing code. I agree with you that I wouldn't go back and change all the files just for the sake of changing it.

Enjoy the Sunday everyone!

@prisae prisae closed this as completed Mar 24, 2019
@leouieda
Copy link
Member

But then again, maybe, I am just too old school

I constantly struggle with these two sides of myself :)

So I close this issue, and I apologize for the tempest in a tea pot.

No need to apologize. It's great that you brought this to my attention and I'll definitely keep it in mind when starting new projects. I hand't even considered that this would be a problem.

I suggest to limit docstrings to 80 characters.

That is a sensible thing to do. From the black repo, it seems like they are working on this. When the implement it, we can take another look at converting the repo to 80 chars again.

@djhoese
Copy link
Contributor

djhoese commented Mar 26, 2019

FYI @prisae this conversation made me rethink my own strategies for some of the libraries I maintain. I'm thinking about switching to 80 characters for docstrings as it is more likely that people will be looking at documentation on smaller screens. We (again, not a verde maintainer) will prefer 80 characters for code but will allow up to 120 characters if it makes sense (it rarely does).

Thanks for bringing this to my attention as well.

@prisae
Copy link
Member Author

prisae commented Mar 26, 2019

Great, thank you both for the discussion @leouieda and @djhoese!

leouieda added a commit to fatiando/community that referenced this issue Nov 7, 2019
leouieda added a commit to fatiando/community that referenced this issue Nov 7, 2019
leouieda added a commit to fatiando/pooch that referenced this issue Nov 14, 2019
Docstrings with 88 characters (the Black line length) don't show up
nicely in Jupyter and IPython (since the display is 80 chars). This
means that our docstrings were unreadable.
See fatiando/verde#177 fatiando/community#9 fatiando/community#10
for context.
Make flake8 check that docstrings (and comments, sadly) are 79
characters so we can make sure we don't do it again accidentally.
Have to check comments as well because the flake8 flag doesn't
discriminate between them.
leouieda added a commit to fatiando/pooch that referenced this issue Nov 14, 2019
Docstrings with 88 characters (the Black line length) don't show up
nicely in Jupyter and IPython (since the display is 80 chars). This
means that our docstrings were unreadable.
See fatiando/verde#177 fatiando/community#9 fatiando/community#10
for context.
Make flake8 check that docstrings (and comments, sadly) are 79
characters so we can make sure we don't do it again accidentally.
Have to check comments as well because the flake8 flag doesn't
discriminate between them.
leouieda added a commit to fatiando/pooch that referenced this issue Nov 18, 2019
Docstrings with 88 characters (the Black line length) don't show up nicely in
Jupyter and IPython (since the display is 80 chars). This means that our
docstrings were unreadable. See fatiando/verde#177 fatiando/community#9
fatiando/community#10 for context. Make flake8 check that docstrings (and
comments, sadly) are 79 characters so we can make sure we don't do it again
accidentally. Have to check comments as well because the flake8 flag doesn't
discriminate between them.

Fixes #121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants