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

Establish & enforce style rules #250

Open
1 of 5 tasks
jlillis opened this issue Nov 13, 2020 · 18 comments
Open
1 of 5 tasks

Establish & enforce style rules #250

jlillis opened this issue Nov 13, 2020 · 18 comments

Comments

@jlillis
Copy link
Contributor

jlillis commented Nov 13, 2020

I think it would be a good idea to establish and begin enforcing (via code reviews and CI checks) style rules for this repository just like we do for mtasa-blue. To this end, I have submitted a PR that adds a .editorconfig file that defines the following basic rules:

  • No trailing whitespace
  • Insert final newlines
  • Use 4 spaces instead of tabs
  • Use CRLF line endings

Line endings

There has been some inconsistency regarding which type of line endings are meant to be used. The .gitattributes file calls for LF endings and it's been mentioned that in the long-term we want LF, but a recent commit seems to have standardized on CRLF. I guess deciding on this would be the first order of buisness.

CI checks

On CI, there have been previous attempts to use lua-fmt to automate formatting checks. It looks like these efforts have stagnated. Perhaps it would be possible to either a) restart work on lua-fmt or b) do something else? I was thinking perhaps we can just run a simple CI check for the style rules I've described above.

Mass-edits and ignore-revs

Once we've decided on a set of rules and implemented the controls to enforce them, another mass-formatting edit will be needed. We should add a .git-blame-ignore-revs file to filter this and previous such commits out.

TLDR

  • establish basic style rules and document them
  • update .editorconfig and .gitattributes accordingly
  • setup related CI checks
  • issue mass-format commit
  • setup .git-blame-ignore-revs
@jlillis
Copy link
Contributor Author

jlillis commented Nov 22, 2020

I have a new rule to propose:

  • Use the global variables root, resource, and resourceRoot rather than calling getRootElement(), getResource(), getResourceRootElement(resource)

@patrikjuvonen
Copy link
Contributor

For additional reference I had worked on a template for styles at https://github.com/multitheftauto/mtasa-resources/wiki/Lua-style-guide

@jlillis
Copy link
Contributor Author

jlillis commented Jan 28, 2021

To get this moving again, can we decide on which line endings are the right line endings? I assume we'd want to use the same style as mtasa-blue, which is CRLF right?

@patrikjuvonen
Copy link
Contributor

Specific style enforcement is required for JS and CSS.

@jlillis
Copy link
Contributor Author

jlillis commented Jan 28, 2021

Specific style enforcement is required for JS and CSS.

Which settings would you like added to a repo editorconfig/prettierrc?

@patrikjuvonen
Copy link
Contributor

patrikjuvonen commented Feb 8, 2021

Specific style enforcement is required for JS and CSS.

Which settings would you like added to a repo editorconfig/prettierrc?

For editorconfig, at least:

[*]
charset = utf-8
end_of_line = crlf
insert_final_newline = true
trim_trailing_whitespace = true

[*.{js,css}]
indent_size = 2
indent_style = space

For prettierrc, at least:

{
  "bracketSpacing": true,
  "singleQuote": true,
  "trailingComma": "all",
  "arrowParens": "always",
  "bracketSameLine": false
}

To get this moving again, can we decide on which line endings are the right line endings? I assume we'd want to use the same style as mtasa-blue, which is CRLF right?

Let's go with CRLF since no one has anything to say and it's not really that important. Can be changed later if needed. I use LF for all my projects elsewhere but in MTA we support only Windows anyway on client so CRLF is "traditional".

Perhaps eslint and stylelint could also be added.

@jlillis jlillis mentioned this issue Feb 16, 2021
@jlillis
Copy link
Contributor Author

jlillis commented Mar 19, 2021

Another new rule in light of #300 - scripts must use client or source appropriately.

@ArranTuna
Copy link
Contributor

I don't understand why you'd want 4 spaces to be used instead of tab when everyone uses tab and there's no benefit to it being 4 spaces? Tabs are specifically designed for indentation and why should everyone have to modify their notepad settings to make it 4 spaces especially if they don't want to use 4 spaces for other projects. Also I just tried converting a script file that uses tabs into 4 spaces and the file size increases 10%.

@AlexTMjugador
Copy link
Member

I don't understand why you'd want 4 spaces to be used instead of tab when everyone uses tab and there's no benefit to it being 4 spaces?

Using tabulation instead of space characters is far from being universal, and there are arguments to favor either approach over the other. I personally prefer tabulation characters too, but if you search over the Internet there is no consensus on what to choose for every project.

What I think that matters most is sticking to a single standard, as mixing both tabs and spaces is arguably the worst approach. In addition to that, doing whatever is more natural, given the default configuration of the editors that the project developers tend to use and the conventions normally used in the broader Lua community, arguably reduces frictions further.

@ArranTuna
Copy link
Contributor

Search results in mtasa-resources for:
tab: mtasa-resources: 198,865 hits
4 spaces: 44,095 hits (divide by 4 = 11,023)

Tab is used to indent 94% of mtasa-resources therefore if there is to be a style rule it should be tab.

@TheNormalnij
Copy link
Member

I've searched for "\n\t" (new line + tab) and "\n " (new line + space)
Tabs: 105059 lines
Spaces: 51619 lines
It looks more realistic

@jlillis
Copy link
Contributor Author

jlillis commented Nov 22, 2021

I don't understand why you'd want 4 spaces to be used instead of tab when everyone uses tab

I don't have a preference either way so long as some standard is enforced. In this case, I just copied what mtasa-blue does.

@Fernando-A-Rocha
Copy link
Contributor

:shipit: Let's make a decision, establish the .editorconfig etc and do the mass style update commit?

@jlillis
Copy link
Contributor Author

jlillis commented Jun 25, 2024

I think it might be good to hold off until the new style rules for mtasa-blue are put together so we can copy whatever is applicable.

@T-MaxWiese-T
Copy link
Contributor

I think it might be good to hold off until the new style rules for mtasa-blue are put together so we can copy whatever is applicable.

I think it makes no sense to wait for it because the coding guidelines are mainly related to C++ code and the coding guidelines would be much less here. I also believe that it makes much more sense for C++ and the large scope. So for code in this repository I haven't really seen the need for coding guidelines yet. This is because Lua is a very simple, compact and undemanding scripting language and so are the MTA functions and events and the structure of the resources.

@Fernando-A-Rocha
Copy link
Contributor

Fernando-A-Rocha commented Jun 25, 2024

for code in this repository I haven't really seen the need for coding guidelines yet. This is because Lua is a very simple, compact and undemanding scripting language and so are the MTA functions and events and the structure of the resources.

There are a few code practices that should be enforced like using early-returns, adequate variable naming etc...

@Fernando-A-Rocha
Copy link
Contributor

Fernando-A-Rocha commented Jun 29, 2024

We could create and maintain the guidelines for contributing to this project here: https://wiki.multitheftauto.com/wiki/Default_resources_-_Contributing (WIP version)

Then, we can proceed with updating the repository with the new style rules, etc.

(#510)

@Fernando-A-Rocha
Copy link
Contributor

Update: coding guidelines for this project are indeed being maintained here
https://github.com/multitheftauto/mtasa-docs/blob/main/mtasa-resources/CONTRIBUTING.md

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

No branches or pull requests

7 participants