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.

Sign up for GitHub

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

Jump to bottom

Cleanup lang #6000

Merged
merged 22 commits into from Jan 1, 2024
Merged

Cleanup lang #6000

merged 22 commits into from Jan 1, 2024

Conversation

sovdeeth
Copy link
Member

Description

I apologize to all reviewers for the length.

This builds on top of #4996.

This PR aims to clean up the lang package of Skript and the base syntax classes.
Main changes are:

  • e -> event and renaming other poorly named variables. (added code convention for this)
  • removing unnecessary finals
  • Fixing typos, removing @ author, and fleshing out poor javadocs
  • Enforcing convention as to annotations and blank lines
  • In SkriptParser#parseSingleExpr, pulled out an instance of duplicate code into a private state method called parseExpression. I'm unsure of this name as there are public methods also named parseExpression.
  • Removes ExpressionList#setAnd

This PR should not change any behavior at all.

Following this PR's merging, I will make a second PR that excludes the changes made in this PR from the git blame. This is done by adding a commit hash into .git-blame-ignore-revs.

Target Minecraft Versions:
Requirements:
Related Issues:

@AyhamAl-Ali AyhamAl-Ali added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 12, 2023
@AyhamAl-Ali
Copy link
Member

How much will you pay per review?

@sovdeeth
Copy link
Member Author

I suppose I could spare a cat picture or two

AyhamAl-Ali
AyhamAl-Ali approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

My payment now!

src/main/java/ch/njol/skript/expressions/base/WrapperExpression.java Outdated Show resolved Hide resolved
@sovdeeth @AyhamAl-Ali
Update src/main/java/ch/njol/skript/expressions/base/WrapperExpressio…
3a6424f 
…n.java

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
@APickledWalrus APickledWalrus changed the base branch from master to dev/feature September 16, 2023 17:03
Moderocky
Moderocky approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

I can't see anything wrong.

src/main/java/ch/njol/skript/lang/UnparsedLiteral.java Show resolved Hide resolved
@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Nov 25, 2023
@APickledWalrus APickledWalrus added the breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) label Dec 16, 2023
APickledWalrus
APickledWalrus requested changes Dec 16, 2023
View reviewed changes
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.

cool beans

src/main/java/ch/njol/skript/lang/SyntaxElementInfo.java Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtGrow.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtGrow.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/DefaultExpression.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/Expression.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/SkriptParser.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/SkriptParser.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/util/SimpleExpression.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/Variable.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/VariableString.java Outdated Show resolved Hide resolved
sovdeeth and others added 4 commits December 15, 2023 18:17
@sovdeeth @APickledWalrus
Apply suggestions from code review
d443f17 
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
@sovdeeth @APickledWalrus
Apply suggestions from code review
131b105 
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
@sovdeeth
Merge branch 'dev/feature' into cleanup-lang
d24ce43
@sovdeeth
requested changes
39ec706
@sovdeeth sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
@sovdeeth sovdeeth merged commit 98abf5d into SkriptLang:dev/feature Jan 1, 2024
4 checks passed
@sovdeeth sovdeeth mentioned this pull request Jan 1, 2024
Merged
@Fusezion Fusezion mentioned this pull request Jan 1, 2024
Merged
@APickledWalrus APickledWalrus mentioned this pull request Jan 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneBeee ShaneBeee ShaneBeee left review comments

@AyhamAl-Ali AyhamAl-Ali AyhamAl-Ali approved these changes

@Moderocky Moderocky Moderocky approved these changes

@APickledWalrus APickledWalrus APickledWalrus approved these changes

Assignees
No one assigned
Labels
2.8 Targeting a 2.8.X version release breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sovdeeth @AyhamAl-Ali @Moderocky @ShaneBeee @APickledWalrus