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

[minor-refactor] Add trailing newline to all text files. #7170

Merged
merged 1 commit into from Jun 30, 2014

Conversation

4 participants
@cirosantilli
Copy link
Contributor

commented Jun 21, 2014

Present in the large majority of files of each respective type.

The only filetype for which we could be not sure is the .txt because there is only one other, but let's follow the other files and do it.

Bit me when doing perl -lapi -e, needed to change to perl -pi -e, or else newlines were added.

Refactoring command used:

git grep -Ile '' | grep -Ev '^vendor' | xargs perl -lapi -e 's/.*/$&/'

-Ile '' to ignore binary files: http://stackoverflow.com/questions/18973057/list-all-text-non-binary-files-in-repo

Add trailing newline to all text files.
Present in the large majority of files of each respective type.
@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2014

I've retriggered the Spinach Mysql test run, since that one had gone wrong.

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2014

It doesn't do any harm, so looks ok to merge i think

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2014

It does seems unrelated, and passed locally.

@dzaporozhets

This comment has been minimized.

Copy link
Member

commented Jun 24, 2014

why we need this? It may make a lot of open PR/MR unmergeable.

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2014

@randx Uniformity implies lower costs. E.g.: can't do perl -lapi -e refactoring: need perl -pi -e and watch out for newlines.

bbastov's style says: "End each file with a newline."

Robocop seems to be enforcing this: https://github.com/bbatsov/rubocop/blob/master/config/default.yml#L407

But no big deal if rejected.

@jvanbaarsen

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2014

@cirosantilli I see what @randx means, we have to ask a lot of people if they want to rebase.
I'll close this one for now. @randx Can you please re-open of you still interested?

@cirosantilli Thanks for all your efforts!

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2014

No problem.

@dosire @jvanbaarsen Can we have a policy or guideline for this and put it on the CONTRIBUTING?

Refactor to follow style guides: yes or no? Refactor for DRYness / better organization yes or no?

Style refactors save time of future contributors, who will edit, try not to touch anything, then get barked at by hound CI or a human reviewer, and then have to change things.

I refactored the entire documentation at: #6863

This is particular issue is not important, but the policy is as it might prevent future contributors and reviewers from doing extra work. =)

@dosire

This comment has been minimized.

Copy link
Member

commented Jun 30, 2014

@cirosantilli Me and @randx discussed this and we welcome this and will merge this. Sorry for the confusion.

@dosire dosire reopened this Jun 30, 2014

dosire added a commit that referenced this pull request Jun 30, 2014

Merge pull request #7170 from cirosantilli/add-trailing-newlines
[minor-refactor] Add trailing newline to all text files.

@dosire dosire merged commit 2f962a4 into gitlabhq:master Jun 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2014

No problem, I hope I didn't annoy you guys with minor things: I really want what is best for this project, and will follow the team's decisions =)

@cirosantilli cirosantilli deleted the cirosantilli:add-trailing-newlines branch Jun 30, 2014

@dosire

This comment has been minimized.

Copy link
Member

commented Jun 30, 2014

@cirosantilli Not annoying at all. We agree that these minor things are important since they prevent Hound CI barking at people. Thanks for taking the time to contribute this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.