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

Breaking change, but minor(?) for countlines() #12844

Closed
wants to merge 2 commits into from

Conversation

PallHaraldsson
Copy link
Contributor

I'm not sure this is the right place to put this. Didn't notice a section on breaking changes. Linked issue, has different title, but its second commit is on this, to docs. Should you not link issues, not that directly? I didn't find another correct issue number. Countlines has parameters, e.g. for the new, for empty lines. Should all (or any) be listed?

I'm not sure this is the right place to put this. Didn't notice a section on breaking changes. Linked issue, has different title, but its second commit is on this, to docs. Should you not link issues, not that directly? I didn't find another correct issue number. Countlines has parameters, e.g. for the new, for empty lines. Should all (or any) be listed?
@pao pao changed the title Breaking change, but minor(?) for countlines() [ci skip] Breaking change, but minor(?) for countlines() Aug 28, 2015
@pao
Copy link
Member

pao commented Aug 28, 2015

...continuing from the other issue, as you may have figured out: putting [ci skip] in the issue title is ineffective. :D

@lostanlen
Copy link

Why should you put commas before and after the words "non-empty lines" ?

@PallHaraldsson
Copy link
Contributor Author

The commas may be overkill or just plain wrong here..

Is this correct: "now also counts non-empty lines, by default"?

with one comma looked wrong to me but maybe isn't.. Similarly, I would say "By default, countlines() counts non-empty-lines".

English (and my native language) use fewer commas than previously (was taught, when I was young..), but I think you are still allowed to use more, just a preference.

If you want "now also counts non-empty lines by default", I think I can change the commit that way.

I' a little confused by:

ARCH="x86_64"

in the Travis CI build fail.

@KristofferC
Copy link
Member

The Travis CI failure is due to running out of memory and it has nothing to do with your change.

I am also not a native speaker but for me it sounds best with no commas.

This one seems important as (default) functionality is different, then "breaking" change. I also have another doc pull request, that seems ok, but could wait.
@nolta nolta closed this in 425fa46 Sep 19, 2015
nolta pushed a commit that referenced this pull request Sep 19, 2015
Missing from #11947
Closes #12844

(cherry picked from commit 425fa46)
@PallHaraldsson
Copy link
Contributor Author

Thanks, I see this was added, but to be useful to people, shouldn't it be added to v0.4.0-rc2? I think (as with similar situations), stuff committed to master, will not show up in 0.4.0* end the eventual released 0.4.0(?).

@tkelman
Copy link
Contributor

tkelman commented Sep 20, 2015

it was backported here ddee103 and will be in rc3

@PallHaraldsson PallHaraldsson deleted the patch-6 branch October 22, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants