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

Code standards update May 2024. #6684

Merged
merged 7 commits into from
Jun 1, 2024

Conversation

Moderocky
Copy link
Member

Description

Our standards committee have decided upon a set of changes to code conventions and contribution standards, designed to make things a little clearer and more modern.

Some former rules have been relaxed (where we no longer felt the restrictions were helpful/necessary) and some new things have been standardised.

Overview of Changes

  1. Annotations
  • We will exclusively use JetBrains's annotations library going forward. We found this to be the most clearly-defined and extensive set of annotations.
  • We will be a little stricter on proper annotation use (in line with JetBrains's provided instructions) to avoid incorrect compiler warnings
    • We will use contracts more
  • We will mandate annotations on arrays in their proper place
  • Annotations for the value (e.g. @NotNull) can now go before the value, rather than the method (e.g. public @NotNull Object myMethod())
  1. Relaxing Ordering Rules
  • We are relaxing the rules around import and annotation ordering a bit (just don't make it look awful, please); we felt it was slowing down pull request lifecycles for little benefit
  1. Licence Header
  • We will no longer require the licence header to be at the start of every file (this should save a few megabytes of space)
  • If a file/package is licensed under a different licence, then its licence will need to go with it
  1. Relaxing Bracket Rules
  • We will be a little more relaxed about {brackets} around single-line blocks.
  • Please don't abuse this or mix brackets and no brackets in the same tree (e.g. if ... else {...})
  1. Ternaries
  • Some ternaries are getting out of hand, and we feel this is making code unclear and illegible
    • Several mistakes were identified recently due to unclear ternary order-of-operations
  • Please avoid nesting multiple (or long) ternaries where it is not necessary
  1. IntelliJ Code Style File
  • We plan to provide an IDE code style file for IntelliJ, which should prevent IDEs violating the rules during automatic reformat
  1. Tests
  • Tests are a good thing, we want more of them!
    • Unit tests for API methods
  • We plan to provide contributing guides on writing tests
  1. Complexity
  • Long methods are okay, illegibly-complex ones are not
  • We are looking at ways to measure method complexity, but please don't be afraid to break long things up into smaller chunks

Target Minecraft Versions: any
Requirements: none
Related Issues: none

@Moderocky Moderocky added enhancement Feature request, an issue about something that could be improved, or a PR improving something. housekeeping Housekeeping-related issue or task. labels May 10, 2024
@Moderocky Moderocky changed the base branch from master to dev/patch May 10, 2024 19:02
@kiip1
Copy link
Contributor

kiip1 commented May 12, 2024

Now might also be a good time to put the .editorconfig file into proper use, by enforcing most of these conventions through it.

This will speed up PRs even further, as developers no longer need to manually adjust their IDE settings for Skript.

Links:

@Moderocky
Copy link
Member Author

Now might also be a good time to put the .editorconfig file into proper use, by enforcing most of these conventions through it.

Tud has already agreed to do this

Copy link
Contributor

@kiip1 kiip1 left a comment

Choose a reason for hiding this comment

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

Sounds good then

code-conventions.md Show resolved Hide resolved
code-conventions.md Show resolved Hide resolved
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

just some thoughts :)

code-conventions.md Show resolved Hide resolved
code-conventions.md Show resolved Hide resolved
code-conventions.md Show resolved Hide resolved
code-conventions.md Show resolved Hide resolved
code-conventions.md Outdated Show resolved Hide resolved
@sovdeeth sovdeeth added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label May 31, 2024
@Moderocky Moderocky merged commit 96ea6b3 into SkriptLang:dev/patch Jun 1, 2024
5 checks passed
@Moderocky Moderocky deleted the update-code-standards branch June 1, 2024 07:26
NotSoDelayed added a commit to NotSoDelayed/Skript that referenced this pull request Jul 19, 2024
NotSoDelayed added a commit to NotSoDelayed/Skript that referenced this pull request Jul 19, 2024
NotSoDelayed added a commit to NotSoDelayed/Skript that referenced this pull request Jul 19, 2024
NotSoDelayed added a commit to NotSoDelayed/Skript that referenced this pull request Jul 19, 2024
sovdeeth added a commit that referenced this pull request Sep 22, 2024
* Add player chat completion suggestions

* Add method exists check

* Syntax adjustment and remove DataFlowIssue suppression

* Docs improvements

* Requested changes

* Update src/main/java/ch/njol/skript/expressions/ExprPlayerChatCompletions.java

Co-authored-by: sovdee <[email protected]>

* Remove license header (#6684)

* Java 17 ready

---------

Co-authored-by: sovdee <[email protected]>
Co-authored-by: Moderocky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. housekeeping Housekeeping-related issue or task. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants