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

coafile: Raise line length limit to 120 #3487

Closed
wants to merge 1 commit into
base: master
from

Conversation

7 participants
@underyx
Member

underyx commented Jan 6, 2017

My suggested policy would be:

  • 80 is good and preferred
  • 100 is okay and no one should really bat an eye at it
  • 120 is bad but justifiable in certain cases, in which cases an ignore comment should be added

Of course, this is just about columns. Having like 10 tokens on one line is still bad and should be avoided, meaning something like this is already bad even though it's just 75 columns:

if result == '{0}'.format(((eggs * float(price)) * people) / len(houses)):

Discuss!

Argument on arguman

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Jan 6, 2017

Collaborator

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

Collaborator

gitmate-bot commented Jan 6, 2017

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 6, 2017

Member

Maybe we should add a code style note in docs, that wrapping at ~100 characters is preferred.

Member

underyx commented Jan 6, 2017

Maybe we should add a code style note in docs, that wrapping at ~100 characters is preferred.

@fneu

This comment has been minimized.

Show comment
Hide comment
@fneu

fneu Jan 6, 2017

Contributor

I'm loosely against this but I'd invite a discussion.

the good:

  • Short line lengths encourage short variable names. This has annoyed me before.
  • a hard requirement encourages messy workarounds. I like the "encourage 100" idea.
  • 80 chars is needlessly short for high levels of indentation

the bad:

  • Long line lengths allow deeper nesting of loops and branch statements. This might encourage more complicated code.
  • 80 characters allows to see two files side-by-side on all kinds of screens. This is personally my biggest reason against this PR tbh. More on this below.
  • 79/80 is apparently enough for PEP8 and Google
  • The codebase is consistent so far

the ugly:

  • we currently advocate 79 characters + newline. We currently enforce 80 characters + newline. It's the first sentence in our codestyle doc. I'd like some consistency after this discussion, please.

on screen sizes:

spectacle ns7606

Here you can see my two 1920x1200 monitors and my 1366x768 laptop. On the big ones I can fit two and three files next to each other (vim, Fira Code 9pt font I believe). On the laptop screen I can still fit two files with readable font size in a conventional editor with sidebar and scrollbars (Kate, Source Code Pro, 8pt)

spectacle ui7606

For comparison: with default settings, Pycharm still allows you to see two 120 char files next to each other on a 1920x____ screen if you don't need a sidebar. It's the same thing with 80 chars on 1366x768 btw. You can decrease the font size to make it fit confortable, it's a little small for my taste though.

spectacle zm7606


Anyway, I didn't want to intimidate anybody... other opinions?

Contributor

fneu commented Jan 6, 2017

I'm loosely against this but I'd invite a discussion.

the good:

  • Short line lengths encourage short variable names. This has annoyed me before.
  • a hard requirement encourages messy workarounds. I like the "encourage 100" idea.
  • 80 chars is needlessly short for high levels of indentation

the bad:

  • Long line lengths allow deeper nesting of loops and branch statements. This might encourage more complicated code.
  • 80 characters allows to see two files side-by-side on all kinds of screens. This is personally my biggest reason against this PR tbh. More on this below.
  • 79/80 is apparently enough for PEP8 and Google
  • The codebase is consistent so far

the ugly:

  • we currently advocate 79 characters + newline. We currently enforce 80 characters + newline. It's the first sentence in our codestyle doc. I'd like some consistency after this discussion, please.

on screen sizes:

spectacle ns7606

Here you can see my two 1920x1200 monitors and my 1366x768 laptop. On the big ones I can fit two and three files next to each other (vim, Fira Code 9pt font I believe). On the laptop screen I can still fit two files with readable font size in a conventional editor with sidebar and scrollbars (Kate, Source Code Pro, 8pt)

spectacle ui7606

For comparison: with default settings, Pycharm still allows you to see two 120 char files next to each other on a 1920x____ screen if you don't need a sidebar. It's the same thing with 80 chars on 1366x768 btw. You can decrease the font size to make it fit confortable, it's a little small for my taste though.

spectacle zm7606


Anyway, I didn't want to intimidate anybody... other opinions?

@fneu fneu requested review from sils and Mixih Jan 6, 2017

@sils

This comment has been minimized.

Show comment
Hide comment
@sils

sils Jan 6, 2017

Member

I think I vote for 105, 120 is too long really and being forced to use short (concise) variable names isn't really much of a bad thing IMO

Member

sils commented Jan 6, 2017

I think I vote for 105, 120 is too long really and being forced to use short (concise) variable names isn't really much of a bad thing IMO

@sils

This comment has been minimized.

Show comment
Hide comment
@sils

sils Jan 6, 2017

Member

105 is essentially the guideline of 100 but you're allowed to overdo it a tiny bit :P

Member

sils commented Jan 6, 2017

105 is essentially the guideline of 100 but you're allowed to overdo it a tiny bit :P

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 6, 2017

Member

Long line lengths allow deeper nesting of loops and branch statements. This might encourage more complicated code.

To prevent this, we should do complexity analysis instead of line length limits.

79/80 is apparently enough for PEP8 and Google

PEP8 is theoretical and Google uses 2 spaces for indentation, so neither is really comparable.

Member

underyx commented Jan 6, 2017

Long line lengths allow deeper nesting of loops and branch statements. This might encourage more complicated code.

To prevent this, we should do complexity analysis instead of line length limits.

79/80 is apparently enough for PEP8 and Google

PEP8 is theoretical and Google uses 2 spaces for indentation, so neither is really comparable.

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 6, 2017

Member

80 characters allows to see two files side-by-side on all kinds of screens. This is personally my biggest reason against this PR tbh. More on this below.

I'd argue that the cutting off or wrapping some characters from the right often results in more readable code than manually breaking them. Compare two versions of what sparked this discussion:

|        regex_keyword = 'r.s.l.'                                                |
|                                                                                |
|        with execute_bear(self.uut, filename='F', file=text,                    |
|                          regex_keyword=regex_keyword,                          |
|                          dependency_results=self.dep_results) as result:       |
|        self.assertEqual(result[0].message, 'The line contains the keyword'     |
|                                            " 'result' which resulted in a"     |
|                                            ' match with given regex.')         |
|                                                                                |
|        text = ['# bla bla bla',                                                |
|                'Issue #123',                                                   |
|                'bla bla bla']                                                  |
|                                                                                |

and

|        regex_keyword = 'r.s.l.'                                                |
|                                                                                |
|        with execute_bear(self.uut, filename='F', file=text,                    |
|                          regex_keyword=regex_keyword,                          |
|                          dependency_results=self.dep_results) as result:       |
|            self.assertEqual(                                                   |
|                result[0].message,                                              |
|                "The line contains the keyword 'result' which resulted in a matc|
|            )                                                                   |
|                                                                                |
|        text = ['# bla bla bla',                                                |
|                'Issue #123',                                                   |
|                'bla bla bla']                                                  |
|                                                                                |
Member

underyx commented Jan 6, 2017

80 characters allows to see two files side-by-side on all kinds of screens. This is personally my biggest reason against this PR tbh. More on this below.

I'd argue that the cutting off or wrapping some characters from the right often results in more readable code than manually breaking them. Compare two versions of what sparked this discussion:

|        regex_keyword = 'r.s.l.'                                                |
|                                                                                |
|        with execute_bear(self.uut, filename='F', file=text,                    |
|                          regex_keyword=regex_keyword,                          |
|                          dependency_results=self.dep_results) as result:       |
|        self.assertEqual(result[0].message, 'The line contains the keyword'     |
|                                            " 'result' which resulted in a"     |
|                                            ' match with given regex.')         |
|                                                                                |
|        text = ['# bla bla bla',                                                |
|                'Issue #123',                                                   |
|                'bla bla bla']                                                  |
|                                                                                |

and

|        regex_keyword = 'r.s.l.'                                                |
|                                                                                |
|        with execute_bear(self.uut, filename='F', file=text,                    |
|                          regex_keyword=regex_keyword,                          |
|                          dependency_results=self.dep_results) as result:       |
|            self.assertEqual(                                                   |
|                result[0].message,                                              |
|                "The line contains the keyword 'result' which resulted in a matc|
|            )                                                                   |
|                                                                                |
|        text = ['# bla bla bla',                                                |
|                'Issue #123',                                                   |
|                'bla bla bla']                                                  |
|                                                                                |
@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 6, 2017

Member

80 characters allows to see two files side-by-side on all kinds of screens. This is personally my biggest reason against this PR tbh. More on this below.

Also, the line breaks often make copy-pasting and visual comparison inconvenient. Again, with the above example, if I wanted to find the difference between a similar string and this one, I'd have a much easier job if I didn't have to compare my one line to the three lines in the code.

And another one: let's say I wrote the message in the test before developing the feature. Now I can start playing around with re-breaking it at words in the actual coalib code if I'm observing a 80 column limit, but I could also just copy-paste it in as it is if our line length limits are more relaxed.

Member

underyx commented Jan 6, 2017

80 characters allows to see two files side-by-side on all kinds of screens. This is personally my biggest reason against this PR tbh. More on this below.

Also, the line breaks often make copy-pasting and visual comparison inconvenient. Again, with the above example, if I wanted to find the difference between a similar string and this one, I'd have a much easier job if I didn't have to compare my one line to the three lines in the code.

And another one: let's say I wrote the message in the test before developing the feature. Now I can start playing around with re-breaking it at words in the actual coalib code if I'm observing a 80 column limit, but I could also just copy-paste it in as it is if our line length limits are more relaxed.

@fneu

This comment has been minimized.

Show comment
Hide comment
@fneu

fneu Jan 6, 2017

Contributor

While I agree that messing with line breaks in text is weird, a longer line length would merely reduce the number of occurrences. The problem persists with strings that are a little longer.

my fear would be that your above snippet would appear as something like:

|        regex_keyword = 'r.s.l.'                                                |
|                                                                                |
|        with execute_bear(self.uut, filename='F', file=text, regex_keyword=regex|
|            self.assertEqual(result[0].message, "The line contains the keyword '|
|                                                                                |
|        text = ['# bla bla bla', 'Issue #123', 'bla bla bla']                   |
|

And while I am cheating here, this could very much be reality with a slightly shorter string and one argument less.

I'm not saying that this is necessarily much worse on a wide screen though:

        regex_keyword = 'r.s.l.'

        with execute_bear(self.uut, filename='F', file=text, regex_keyword=regex_keyword, dependency_results=self.dep_results) as result:
            self.assertEqual(result[0].message, "The line contains the keyword 'result' which resulted in a match with given regex.")

        text = ['# bla bla bla', 'Issue #123', 'bla bla bla']

Contributor

fneu commented Jan 6, 2017

While I agree that messing with line breaks in text is weird, a longer line length would merely reduce the number of occurrences. The problem persists with strings that are a little longer.

my fear would be that your above snippet would appear as something like:

|        regex_keyword = 'r.s.l.'                                                |
|                                                                                |
|        with execute_bear(self.uut, filename='F', file=text, regex_keyword=regex|
|            self.assertEqual(result[0].message, "The line contains the keyword '|
|                                                                                |
|        text = ['# bla bla bla', 'Issue #123', 'bla bla bla']                   |
|

And while I am cheating here, this could very much be reality with a slightly shorter string and one argument less.

I'm not saying that this is necessarily much worse on a wide screen though:

        regex_keyword = 'r.s.l.'

        with execute_bear(self.uut, filename='F', file=text, regex_keyword=regex_keyword, dependency_results=self.dep_results) as result:
            self.assertEqual(result[0].message, "The line contains the keyword 'result' which resulted in a match with given regex.")

        text = ['# bla bla bla', 'Issue #123', 'bla bla bla']

@Mixih

This comment has been minimized.

Show comment
Hide comment
@Mixih

Mixih Jan 9, 2017

Member

I feel that 120 is really a bit too long. I like to have a side bar and a gutter when comparing two files, and auto line breaks tend to interrupter the flow of code on the editors I use. I am not too opposed to increasing the line length to 100, however, this can open further opportunity for line length expansion. I think we should stress that keeping variable names descriptive while being short is preferred over a really long line with overly verbose variables.

Member

Mixih commented Jan 9, 2017

I feel that 120 is really a bit too long. I like to have a side bar and a gutter when comparing two files, and auto line breaks tend to interrupter the flow of code on the editors I use. I am not too opposed to increasing the line length to 100, however, this can open further opportunity for line length expansion. I think we should stress that keeping variable names descriptive while being short is preferred over a really long line with overly verbose variables.

@yukiisbored

This comment has been minimized.

Show comment
Hide comment
@yukiisbored

yukiisbored Jan 10, 2017

Member

120 is too big IMHO, The problem with this is for people who are mostly using laptops/netbooks, most laptops/netbooks today uses 1366x768 (In fact, it's the most used screen resolution). With 80/100, I can view two files side-by-side on emacs and other text editors as well. I personally program and do stuff at school (because I'm currently a senior high school student), so It's not common for me to have the exclusive 1920x* resolution goodness. Also with 80/100, I can fit more stuff like other source files, documentations, videos, and some other stuff while programming as well. 100 causes some line-wrapping for me but that's okay because they're small IMHO.

emacs viewing two files side-by-side
www

emacs and a Firefox window side-by-side
docs

Font used: Hack 10pt

Member

yukiisbored commented Jan 10, 2017

120 is too big IMHO, The problem with this is for people who are mostly using laptops/netbooks, most laptops/netbooks today uses 1366x768 (In fact, it's the most used screen resolution). With 80/100, I can view two files side-by-side on emacs and other text editors as well. I personally program and do stuff at school (because I'm currently a senior high school student), so It's not common for me to have the exclusive 1920x* resolution goodness. Also with 80/100, I can fit more stuff like other source files, documentations, videos, and some other stuff while programming as well. 100 causes some line-wrapping for me but that's okay because they're small IMHO.

emacs viewing two files side-by-side
www

emacs and a Firefox window side-by-side
docs

Font used: Hack 10pt

@fneu

This comment has been minimized.

Show comment
Hide comment
@fneu

fneu Jan 11, 2017

Contributor

I think 120 is out of the question, but a lot of people seem to be ok with 100.
How do we now proceed and come to a decision here?
I'm still for 80 anyway :p

Contributor

fneu commented Jan 11, 2017

I think 120 is out of the question, but a lot of people seem to be ok with 100.
How do we now proceed and come to a decision here?
I'm still for 80 anyway :p

@sils

This comment has been minimized.

Show comment
Hide comment
@sils

sils Jan 15, 2017

Member

I hereby vote for 80 characters because it leads to less complicated functions and I can fit more stuff on my screen. I don't feel that this is painful since I'm accustomed to it and I think this has a positive effect on quality.

Member

sils commented Jan 15, 2017

I hereby vote for 80 characters because it leads to less complicated functions and I can fit more stuff on my screen. I don't feel that this is painful since I'm accustomed to it and I think this has a positive effect on quality.

@jayvdb

This comment has been minimized.

Show comment
Hide comment
@jayvdb

jayvdb Jan 16, 2017

Member

I agree on sticking with the current convention, at least until we have complexity analysis enforced.
I have no preference re 79+\n or 80+\n, but agree we should pick one and stick to it.

Member

jayvdb commented Jan 16, 2017

I agree on sticking with the current convention, at least until we have complexity analysis enforced.
I have no preference re 79+\n or 80+\n, but agree we should pick one and stick to it.

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Jan 23, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Jan 23, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 23, 2017

Member

@kennethreitz just published this: https://www.kennethreitz.org/essays/if-i-could-amend-pep-8

Doesn't erally elaborate on the argument, but hey, he agrees with me so I'll mention it :D

Member

underyx commented Jan 23, 2017

@kennethreitz just published this: https://www.kennethreitz.org/essays/if-i-could-amend-pep-8

Doesn't erally elaborate on the argument, but hey, he agrees with me so I'll mention it :D

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 28, 2017

Member

And here's another case against the 80 limit, it causes people to write stuff like these tests:

self.assertEqual(collect_files([os.path.join(self.collectors_test_dir,
                                             'others',
                                             '*',
                                             '*2.py')],
                               self.log_printer),
                 [os.path.normcase(os.path.join(
                     self.collectors_test_dir,
                     'others',
                     'py_files',
                     'file2.py'))])

instead of something like

self.assertEqual(
    collect_files([os.path.join(self.collectors_test_dir, 'others', '*', '*2.py')], self.log_printer),
    [os.path.normcase(os.path.join(self.collectors_test_dir, 'others', 'py_files', 'file2.py'))],
)
Member

underyx commented Jan 28, 2017

And here's another case against the 80 limit, it causes people to write stuff like these tests:

self.assertEqual(collect_files([os.path.join(self.collectors_test_dir,
                                             'others',
                                             '*',
                                             '*2.py')],
                               self.log_printer),
                 [os.path.normcase(os.path.join(
                     self.collectors_test_dir,
                     'others',
                     'py_files',
                     'file2.py'))])

instead of something like

self.assertEqual(
    collect_files([os.path.join(self.collectors_test_dir, 'others', '*', '*2.py')], self.log_printer),
    [os.path.normcase(os.path.join(self.collectors_test_dir, 'others', 'py_files', 'file2.py'))],
)
@yukiisbored

This comment has been minimized.

Show comment
Hide comment
@yukiisbored

yukiisbored Jan 30, 2017

Member

One code is horrifying and the other one is more horrifying. A simple solution would be to move them into their own variables. I really don't see why the 80 columns rule is a problem here, because for almost anything, you can find a way to make them fit while keeping them readable.

The main problem with >80 is a bad idea because it promotes deeper nested code, like for loops, if statements, etc. When the project has a 80 column rule, you are forced to simplify your code. Split them into their own variables, functions, etc and decreasing code complexity, ABC, etc in the process. I really don't see why readability is such a problem, because from what I see you can just split them into their own variables or functions instead of forcing them to be in one line.

Member

yukiisbored commented Jan 30, 2017

One code is horrifying and the other one is more horrifying. A simple solution would be to move them into their own variables. I really don't see why the 80 columns rule is a problem here, because for almost anything, you can find a way to make them fit while keeping them readable.

The main problem with >80 is a bad idea because it promotes deeper nested code, like for loops, if statements, etc. When the project has a 80 column rule, you are forced to simplify your code. Split them into their own variables, functions, etc and decreasing code complexity, ABC, etc in the process. I really don't see why readability is such a problem, because from what I see you can just split them into their own variables or functions instead of forcing them to be in one line.

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 30, 2017

Member

@yukiisbored @sils @fneu

You keep saying that a limit of 80 discourages people from writing deeply nested code, but is there actually any evidence of this?

The snippets I brought up above were written without any regard to readability or thinking about proper styling. So what do you think will happen if at 5 indentation levels devs have access to only 60 columns? My guess would be they'd just keep splattering the code with those reckless newlines without a second thought that we can see above as well.

Member

underyx commented Jan 30, 2017

@yukiisbored @sils @fneu

You keep saying that a limit of 80 discourages people from writing deeply nested code, but is there actually any evidence of this?

The snippets I brought up above were written without any regard to readability or thinking about proper styling. So what do you think will happen if at 5 indentation levels devs have access to only 60 columns? My guess would be they'd just keep splattering the code with those reckless newlines without a second thought that we can see above as well.

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Jan 30, 2017

Member

I guess my point with this is:

When the project has an 80 column rule, you are forced to simplify your code

This only applies if you already have the sort of developer's intuition that makes you groan when you look at this code. Otherwise, this benefit just doesn't exist.

But for those who do have this intuition, they also have the same intuition that allows them to reasonable utilize 100-120 columns. For these people, there's just no point in artificially tying their hands with a column limit.

Member

underyx commented Jan 30, 2017

I guess my point with this is:

When the project has an 80 column rule, you are forced to simplify your code

This only applies if you already have the sort of developer's intuition that makes you groan when you look at this code. Otherwise, this benefit just doesn't exist.

But for those who do have this intuition, they also have the same intuition that allows them to reasonable utilize 100-120 columns. For these people, there's just no point in artificially tying their hands with a column limit.

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Feb 1, 2017

Member

Two more small things:

  • Not breaking strings up every few words to fit into <80 columns makes it way easier to search the code (for instance when you're trying to find where an error message comes from).
  • Having strings broken up so much makes git diffs totally unreadable, and makes changing the text very tedious, discouraging people from improving wording and clarifying text.
Member

underyx commented Feb 1, 2017

Two more small things:

  • Not breaking strings up every few words to fit into <80 columns makes it way easier to search the code (for instance when you're trying to find where an error message comes from).
  • Having strings broken up so much makes git diffs totally unreadable, and makes changing the text very tedious, discouraging people from improving wording and clarifying text.
@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Feb 1, 2017

Member

This discussion is getting a bit too long, so I've created an argument on arguman. Can we move everything there?

Member

underyx commented Feb 1, 2017

This discussion is getting a bit too long, so I've created an argument on arguman. Can we move everything there?

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Feb 8, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Feb 8, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Feb 19, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Feb 19, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Feb 26, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Feb 26, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@underyx

This comment has been minimized.

Show comment
Hide comment
@underyx

underyx Mar 1, 2017

Member

I'm unsubscribing from this thread due to the gitmate spam. If there's any activity, please tag me to send me a notification.

Member

underyx commented Mar 1, 2017

I'm unsubscribing from this thread due to the gitmate spam. If there's any activity, please tag me to send me a notification.

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Mar 8, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Mar 8, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Mar 16, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Mar 16, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Mar 23, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Mar 23, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Mar 30, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Mar 30, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Apr 6, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Apr 6, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot

This comment has been minimized.

Show comment
Hide comment
@gitmate-bot

gitmate-bot Apr 13, 2017

Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Collaborator

gitmate-bot commented Apr 13, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@jayvdb

This comment has been minimized.

Show comment
Hide comment
@jayvdb

jayvdb Apr 14, 2017

Member

http://en.arguman.org/it-is-best-to-not-enforce-an-80-column-width-limit-in-python is really useful, but IMO this PR is not useful.

It seems there is a clear majority that adding complexity analysis is required before the line length is increased, so this is definitely status/blocked. Luckily we have the tools to do that easily, and we have bugs raised for it, but it will take a concerted effort to do it, and someone needs to drive it, so .. closing .. as it is blocked on a very large project, and keeping a one line PR open but blocked indefinitely on a large/untriaged project is unhelpful.

The PR is by a maintainer, so they can re-open when the time is right ;-) Or my decision reversed by the BDFL, in which case please actually merge this.

Maybe this can be done first in the smaller repos. I would support it in https://gitlab.com/coala/package_manager/ , which has terrifically long import paths and class names which make tests a bit interesting.

Member

jayvdb commented Apr 14, 2017

http://en.arguman.org/it-is-best-to-not-enforce-an-80-column-width-limit-in-python is really useful, but IMO this PR is not useful.

It seems there is a clear majority that adding complexity analysis is required before the line length is increased, so this is definitely status/blocked. Luckily we have the tools to do that easily, and we have bugs raised for it, but it will take a concerted effort to do it, and someone needs to drive it, so .. closing .. as it is blocked on a very large project, and keeping a one line PR open but blocked indefinitely on a large/untriaged project is unhelpful.

The PR is by a maintainer, so they can re-open when the time is right ;-) Or my decision reversed by the BDFL, in which case please actually merge this.

Maybe this can be done first in the smaller repos. I would support it in https://gitlab.com/coala/package_manager/ , which has terrifically long import paths and class names which make tests a bit interesting.

@jayvdb jayvdb closed this Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment