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

Condition cleanup #1597

Merged
merged 3 commits into from
Oct 12, 2018
Merged

Condition cleanup #1597

merged 3 commits into from
Oct 12, 2018

Conversation

Nicofisi
Copy link
Member

Copy-pasted from the message of the first commit (only reformatted line endings):

  • changed some conditions to property conditions, where it made sense
  • replaced new Checker() { ... } with lambdas
  • utilized the static toString method of the PropertyCondition class in a few non-property conditions where it made sense, for guaranteed toString consistency and proper grammar
  • in nested Expression#check calls, put the negation in the outer call, because this way it makes more sense logically. For some 'proof', see the CondCompare class written by Njol, it has always had the the isNegated() parameter only in the outer call. I've considered this deeply myself too and I am greatly convicted it makes more sense this way
  • changed the method order in a few classes
  • fixed up the nullability annotations in a few classes
  • added tabs on empty lines in some files where they were missing (we plan to remove them afaik, but for now for consistency)
  • added or removed empty lines in a few places, for consistency and readability
  • suppressed a deprecation warning in one place where it was clear that we're aware that the method was deprecated although we still need to use it to support older Minecraft versions
  • added assert false;s before return false;s that should never be run

- changed some conditions to property conditions, where it made sense
- replaced `new Checker() { ... }` with lambdas
- utilized the static toString method of the PropertyCondition
  class in a few non-property conditions where it made sense,
  for guaranteed toString consistency and proper grammar
- in nested Expression#check calls, put the negation in the outer
  call, because this way it makes more sense logically.
  For some 'proof', see the CondCompare class written by Njol,
  it has always had the the `isNegated()` parameter only in the
  outer call. I've considered this deeply myself too and I am
  greatly convicted it makes more sense this way
- changed the method order in a few classes
- fixed up the nullability annotations in a few classes
- added tabs on empty lines in some files where they were missing
  (we plan to remove them afaik, but for now for consistency)
- added or removed empty lines in a few places, for consistency
  and readability
- suppressed a deprecation warning in one place where it was
  clear that we're aware that the method was deprecated
  although we still need to use it to support older Minecraft versions
- added `assert false;`s before `return false;`s that should
  never be run
# Conflicts:
#	src/main/java/ch/njol/skript/conditions/CondCanFly.java
#	src/main/java/ch/njol/skript/conditions/CondCanSee.java
#	src/main/java/ch/njol/skript/conditions/CondHasClientWeather.java
@Nicofisi Nicofisi added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Oct 12, 2018
@Nicofisi Nicofisi merged commit ef787bf into master Oct 12, 2018
@Nicofisi
Copy link
Member Author

Long story short: the 'fetch' button in the official GitHub Desktop app is for some reason broken and it pushed the commits to master instead... feel free to make comments about any aspect of the commits and I will reply to all of them or even revert the whole thing if necessary... "thanks GitHub", and sorry again to everyone

@bensku
Copy link
Member

bensku commented Oct 12, 2018

How stable is this?

@Nicofisi
Copy link
Member Author

I've read all the changes line be line three times before committing, and 99% of them were just simple reformats or condition -> property condition updates, so I'm quite confident this should be stable... I'll build a jar and ask a few people to confirm the whole thing in action in real scripts ASAP though, and will report back either today or the day after tomorrow

@Nicofisi Nicofisi deleted the condition-cleanup branch October 12, 2018 19:53
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants