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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalizing final new lines #1744

Merged
merged 3 commits into from Aug 16, 2017
Merged

Normalizing final new lines #1744

merged 3 commits into from Aug 16, 2017

Conversation

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Aug 13, 2017

馃帺 What? Why?

This is me experimenting with normalizing final new lines inside the repo, so that:

  • They don't add unnecessary noise to diffs.
  • We get correct line counts if we ever need stats about Decidim LOCs :bowtie:.

I'm not sure if this will work, but wanted to try.

馃搶 Related Issues

None.

馃搵 Subtasks

None.

馃摲 Screenshots (optional)

None.

馃懟 GIF

esquivando

@codecov
Copy link

@codecov codecov bot commented Aug 13, 2017

Codecov Report

Merging #1744 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1744   +/-   ##
=======================================
  Coverage   98.61%   98.61%           
=======================================
  Files         970      970           
  Lines       22214    22214           
=======================================
  Hits        21906    21906           
  Misses        308      308

@ghost ghost added the in-progress label Aug 13, 2017
@@ -1 +1,3 @@
* text=auto
*/vendor/* -text
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think this is going to work for final new lines, I think it only handles new line style (lf vs crlf).

Copy link
Contributor

@beagleknight beagleknight left a comment

I am used to ending all my files with a new line but you made some good points here :)

timepicker: "%d/%m/%Y %H:%M"
Copy link
Contributor

@josepjaume josepjaume Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All things related to translations will probably break thanks to Crowdin :(

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was hoping that the git config added to .gitattributes would enforce that crowdin actually commits the correct final new line. But I don't think that's the case after all.

I'm not sure this patch is worth at the moment since we have no way to enforce the consistency in the future. If you still see value, I can remove the changes to both .gitattributes and config/locales, otherwise just close.

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, we briefly discussed this here. @mrcasals Did you get a reply from crowdin?

Copy link
Contributor

@mrcasals mrcasals Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get a clear answer from them. They said they use single quotes, but nothing more. Will try to press a little more on the subject.

Copy link
Contributor

@mrcasals mrcasals Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, they said:

According to YAML specification, all strings that contain special characters (?, 驴, [], {} etc.) should be wrapped with the quotes.

But as far as I can understand, it doesn't prefer one type of quotes over the other one 馃槙

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in the spec the talk about "quote styles": http://www.yaml.org/spec/1.2/spec.html#id2786942, nothing compulsory or even recommended.

I'm fine with using single quotes for YAML files when possible (although I'd prefer to be able to choose). What really feels weird is that they choose a "no final newline" style, since that's clearly minoritary.

@josepjaume josepjaume dismissed stale reviews from mrcasals and beagleknight via 57dcd67 Aug 16, 2017
@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Aug 16, 2017

@deivid-rodriguez I'd merge this now as it fixes a lot of consistency errors. Let's see how it behaves in the future.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Aug 16, 2017

Sounds good, we can also add an .editorconfig file to try make it easier (or even transparent for some editors) for contributors to follow this.

Shall I remove the changes in config/locales, or just merge and see what crowdin does?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Aug 16, 2017

Something like this, for example:

root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Aug 16, 2017

I say merge this and let's see what crowdin does. Didn't know about .editorconfig, cool!

@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Aug 16, 2017

Will you add the .editorconfig file then?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Aug 16, 2017

Yeah, hold on!

I used the following script

```bash
for file in $(git grep --cached -Il '' | grep -v /vendor/)
do
  if [ -n "$(tail -c 1 < "$file")" ]
  then
    echo >> "$file"
  fi
done

```
To try to minimize diff noise caused by different editors.
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Aug 16, 2017

@josepjaume It's ready 馃帀

@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Aug 16, 2017

Nice!

@josepjaume josepjaume merged commit 532e149 into master Aug 16, 2017
4 checks passed
@josepjaume josepjaume deleted the fix/final_new_lines branch Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants