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 .editorconfig file #247

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jlillis
Copy link
Contributor

@jlillis jlillis commented Nov 10, 2020

When merged, this will add an .editorconfig file to the repo to help enforce a consistent coding style. The following rules are set:

  • Add final newline and trim trim trailing whitespace from all files
  • Use 4 tabs instead of spaces and crlf line endings in .lua files

@jlillis jlillis mentioned this pull request Nov 10, 2020
Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

I think in the long term we'll want to standardise on LF (as we have in mtasa-blue), but for now, since CRLF is the most common, we can stick with this.

In the future, we should have some sort of formatter that enforces all Lua formatting. We can change all CRLF to LF in one fell sloop, along with the Lua formatting enforcer.


Extra context about formatting: there are/were dreams of adding a Lua formatter that will enforce all Lua code to be formatted a certain way. We just needed to finish off trixnz/lua-fmt#21 and then apply it to this repo. Time and priorities meant that I never got round to this. Also, that project was abandoned even when I commented on that PR. That said, we applied lua-fmt to the admin2 resource, as a proof of concept.

We didn't apply it to the entire repository as we wanted to make sure we do only one mass formatting commit, instead of multiple over the years. (Unfortunately, it looks like this commit 2c6416a slipped through.)

@jlillis
Copy link
Contributor Author

jlillis commented Nov 10, 2020

Did commit 2c6416a standardize the repo on LF or CRLF? If LF, then I should update my settings here I think. Also, is there any way to perform a "simpler" format checking at just checks for proper indentation and line endings? That seems to be the most common pain point.

@qaisjp
Copy link
Contributor

qaisjp commented Nov 10, 2020

Unfortunately I think it accidentally accidentally changed it to CRLF. It seems to edit all admin2 files, which iirc were previously standardised to LF by lua-fmt. I'm going to turn on pull request enforcements so we can catch this in code review next time.

@qaisjp
Copy link
Contributor

qaisjp commented Nov 10, 2020

Also, is there any way to perform a "simpler" format checking at just checks for proper indentation and line endings?

I think we could do that. Do you want to look into that, or shall I?

@jlillis
Copy link
Contributor Author

jlillis commented Nov 11, 2020

I think we could do that. Do you want to look into that, or shall I?

I have zero experience with GitHub Actions/CI so it's probably best if you look into it.

Looks like you're correct: .gitattributes calls for LF line endings.

If we can get consensus on the settings I've defined so far I'll go ahead and add a CONTRIBUTING.md file describing the desired coding style so far.

A seperate PR to format existing code is probably called for too.

@qaisjp
Copy link
Contributor

qaisjp commented Nov 11, 2020

Oh, hm, if our .gitattributes calls for LF, and most of our current code is CRLF, I'm not sure what to do.

Maybe we can just comment out that line of the .gitattributes file, and revisit line endings (and mass-formatting) later.

@jlillis
Copy link
Contributor Author

jlillis commented Nov 13, 2020

I think that's probably the best move for now. I'll open a separate issue for discussion.

@jlillis jlillis mentioned this pull request Nov 13, 2020
5 tasks
.editorconfig Outdated
[*.lua]
indent_style = space
indent_size = 4
end_of_line = crlf
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should comment this out too? The repo is currently mixed, right?

@jlillis jlillis marked this pull request as draft November 17, 2020 16:00
@jlillis
Copy link
Contributor Author

jlillis commented Nov 17, 2020

Going to switch this back to a draft PR until we make some more decisions in #250

@Fernando-A-Rocha
Copy link
Contributor

I think it's safe to implement this now

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

Successfully merging this pull request may close these issues.

4 participants