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

Add a standard configuration to every contributor follow #649

Merged

Conversation

brunoocasali
Copy link
Contributor

Context

When I delivering some pull requests like #648 and #647 I've noticed about my editor not removing trailing spaces to me, then I realized this project have no standard configuration about editor configuration. So, this PR addresses this improvement!

Summary of Changes

  • Added editorconfig

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 17, 2020

Quick comment: We are looking at adopting the Rubocop linter for Ruby code in #644. Hopefully this is consistent with what Rubocop does? (For example: the amount of spacing for indentation).

@brunoocasali
Copy link
Contributor Author

brunoocasali commented Oct 23, 2020

Quick comment: We are looking at adopting the Rubocop linter for Ruby code in #644. Hopefully this is consistent with what Rubocop does? (For example: the amount of spacing for indentation).

Yes @DeeDeeG, rubocop and editorconfig can live in the same project! Actually editorconfig can prevent many offenses that will be raised by rubocop :)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 23, 2020

@brunoocasali do you mind if this is the config?

root = true

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

I think it's simpler to apply the rules to all files. And it looks like the markdown/md rule may have been auto-generated, but I personally think it's a good idea to add a newline at the end of markdown files.

I have tried this configuration in Atom, with the editorconfig package. It seems to work fine with all our existing files.

@DeeDeeG DeeDeeG added developer-oriented Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) Ready for Review labels Oct 23, 2020
.editorconfig Outdated
Comment on lines 1 to 12
root = true

[*]
insert_final_newline = true
trim_trailing_whitespace = true

[*.{rb,yml}]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8

[*.{md,markdown}]
trim_trailing_whitespace = false
Copy link
Contributor

@DeeDeeG DeeDeeG Oct 23, 2020

Choose a reason for hiding this comment

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

If you agree with applying these rules to all files, this comment allows applying the change with one button press. Feel free to ignore this if you rather update it on command-line. And I'm completely open to discussion/your thoughts on which rules should apply to which files.

Suggested change
root = true
[*]
insert_final_newline = true
trim_trailing_whitespace = true
[*.{rb,yml}]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
[*.{md,markdown}]
trim_trailing_whitespace = false
root = true
[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

Best regards,

- DeeDeeG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this Github feature is awesome!

I'm just concerned about the markdown files, because markdown uses trailing spaces to deal with line breaking and other rendering stuff.
And I agree with u about making it more simpler, (less is more!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about:

root = true

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

[*.{md,markdown}]
trim_trailing_whitespace = false

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for bringing that to my attention. I am surprised to hear that trailing whitespace can be meaningful in MarkDown files.

That having been said, the three MarkDown files in this repository (CODE_OF_CONDUCT.md CONTRIBUTING.md, and README.md) currently do not use trailing whitespace to mean anything (I think our MarkDown files do not have any trailing whitespace at all?), and I personally prefer that no trailing whitespace should be used in any files, even in MarkDown files.

If EditorConfig can make that happen, then that's great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DeeDeeG by default, two spaces in markdown is the same as a <br>, so even if our files don't use that today doesn't mean that never will be used 😅 (to me keeping that config is a "future proof", that prevents someone have some trouble with this)

Having said, if you still prefer to remove trailing spaces by editorconfig let me know and I will change 🥇

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, I think this is ready to merge, then.

If you want my thoughts on trailing spaces, you can read on. I didn't want to seem arbitrary by not explaining my views. But ultimately these comments are not needed to be addressed. They are just my thoughts.


I greatly prefer:

  • Start a new paragraph <p> HTML element, by typing two newlines, AND/OR
  • Transition to a new kind of content, whether it be a header, paragraph text, or a code block

Very distant second place:

  • Indicate a line break with <br> or a trailing \
    • (potentially formats new lines within the same paragraph)

Very, very distant third place (please, folks, don't do this):

  • Indicate a line break with two or more trailing spaces
    • (potentially formats new lines within the same paragraph)

We have been using either new paragraphs (indicated with two newlines), or else a transition to a new type of content (header to paragraph text, paragraph text to a code block) to induce line formatting. This has been adequate for our formatting needs, and is both intuitive to read, and in my opinion unambiguous/not confusing for the reader. Simple is good.

This also makes the spacing of the document more uniform, and hopefully easier to skim. The situations where a newline is needed, within the same paragraph as previous lines of text, are rare, and so far we have not needed this.

And MarkDown handles these newlines within a paragraph in a way that isn't great:

  • Even intentional trailing spaces could be ambiguous in their intent vs a typo accidentally included in the file. This makes a pull request a bit harder to review, and is a source of potential friction in the review communication process. ("Was this a typo? Can you use <br> instead?")
  • <br> elements look awkward, and in my opinion it's ideal not to use <>-style HTML elements in MarkDown, if it can be avoided.
  • Trailing slashes \ are not intuitive or similar to any other formatting characters I've seen in other languages. In the command-line, trailing slashes nullify the meaning of newlines, not preserve them. The choice doesn't make much sense to me. This should also be avoided, in my opinion.

(Ideally, single newline characters such as LF should have been respected, not ignored. They should have been treated as literal newlines in formatting the document. This is how newlines are interpreted in GitHub comments, and it makes sense to me.)

That said, it is a valid part of the language to use two trailing spaces to indicate a newline. So just in the interest of not confusing people who are used to that, I suppose trailing spaces should be allowed...

It would have been my preference for trailing whitespace not to be a meaningful part of the language. I personally hope contributors will not use it. I'm not the only maintainer here, though, and the other maintainers have not commented yet, so I'll allow it.

@DeeDeeG DeeDeeG merged commit 82ef4c4 into RefugeRestrooms:develop Oct 26, 2020
@DeeDeeG DeeDeeG mentioned this pull request Oct 31, 2020
5 tasks
DeeDeeG added a commit that referenced this pull request Nov 1, 2020
* Finish Upgrading Webpacker to v5 (#637)
  - Gemfile[.lock]: Update webpacker to 5.2.1
  - Webpacker: Update config files
      Ran "rails webpacker:install" and committed the changes.

      In "config/webpacker.yml,"
      kept the ".js.erb" entry under "extensions:"
      so that "app/javascript/packs/lib/maps.js.erb" is included.

      In "config/webpack/environment.js,"
      kept jQuery and rails-erb-loader configs,
      as we are still using these.
  - JS dependencies: Upgrade webpack-dev-server
      Commit the changes from running `rails webpacker:install`,
      but using a caret "^" semver range for "@rails/webpacker",
      rather than an exact version.

      (So that we get updates in the 5.x series)


* Update dependencies for September 2020 (#636)
  - dependencies: Remove `swagger` node module
      We don't actually use this.

      Also removes 200 sub-dependencies,
      and makes our yarn.lock file about 1270 lines shorter!
  - yarn.lock: Upgrade bootstrap to 4.5.2
      (Was at version 4.4.1)
  - dependencies: Resolve node-fetch to ^2.6.1
  - yarn.lock: Upgrade "selfsigned" and "node-forge"
  - Gemfile[.lock]: Update Rails and dependencies
      rails was 5.2.4.3, now it's version 5.2.4.4


* Keeping filters state on pagination (#638)
  - Keeping filters state on pagination
  - Fix Code Climates complaints.
  - Fixed active issue on ada filter buttons
  - Fixed undefined function problem when run rspec.


* Add rubocop and resolve lint errors (#644)
  - Initialize rubocop
  - Run rubocop with --fix
  - Don't require magic comment
      This will affect every file and have potential side effects, so I'm
      going to start with this turned off.
  - Resolve lint errors in `app`
  - RSpec linting
  - Resolve remaining
  - Fix wrong change
  - Singleton method for verify
  - New lint rules
  - Add rubocop to travis config
  - Correct docker compose command
  - Check for geo
      The condition was actually supposed to be `if geo = results.first`,
      because that's not always obvious the intention and fails lint, I'm just
      doing a truthy check for it which should be the same.
  - Fix typo
  - Add contributing docs for rubocop


* Applying ADA filters for the Map view of search. (#651)
  - Applying ADA filters for the Map view of search.
  - Changed comment for polyfill URLSearchParams
  - Fixed bug of map marker content not showing.


* Add a standard EditorConfig configuration for every contributor to follow (#649)


* Enhancement/i18n thread safe (#647)
  - Provide a way to respond requests without thread-safe issues
      As rails docs says https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests
      When we use 'I18n.locale =' the current locale could leak into following requests
      this is a standard and recommended way to deal with it :)
  - Create a shared_example to test I18n locale switching
  - Update spec/controllers/pages_controller_spec.rb


* Upgrade puma to latest version 5.x (#641)


* yarn.lock: Update "http-proxy" to v1.18.1 (fe195d7)


Co-authored-by: Lucas <torres.giorgio@gmail.com>
Co-authored-by: Tegan Rauh <3896310+tegandbiscuits@users.noreply.github.com>
Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-oriented Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants