-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Feature: Block Opening /cf
Without Hot Chocolate Mixin
#3024
Open
DavidArthurCole
wants to merge
23
commits into
hannibal002:beta
Choose a base branch
from
DavidArthurCole:CfChocMixinDetect
base: beta
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
ee8f633
Done
DavidArthurCole 743152d
Update there too
DavidArthurCole fe4e7da
Push
DavidArthurCole 93e063a
Sure
DavidArthurCole 4b649a2
+
DavidArthurCole 871c9a8
Detekt
DavidArthurCole 06f6fac
Merge branch 'beta' into CfChocMixinDetect
CalMWolfs f9b3830
Logical fix
DavidArthurCole da3a05b
Requested changes
DavidArthurCole ab3b58a
Imps
DavidArthurCole e86f00b
Merge branch 'hannibal002:beta' into CfChocMixinDetect
DavidArthurCole 68d110d
Refactor to API
DavidArthurCole 40c0ec2
Detekt
DavidArthurCole f389409
Merge
DavidArthurCole 13f28e0
Fixes
DavidArthurCole 9b41ecf
Merge branch 'hannibal002:beta' into CfChocMixinDetect
DavidArthurCole c090f10
Merge branch 'hannibal002:beta' into CfChocMixinDetect
DavidArthurCole 362c4f8
Merge branch 'refs/heads/beta' into fork/DavidArthurCole/CfChocMixinD…
CalMWolfs 2851344
fix merge
CalMWolfs 5ee6d40
Create package
DavidArthurCole 18e8db6
Detekt
DavidArthurCole 80c453d
Merge branch 'hannibal002:beta' into CfChocMixinDetect
DavidArthurCole 5c02de5
Merge branch 'hannibal002:beta' into CfChocMixinDetect
DavidArthurCole File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesnt look right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate more please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have not looked at this in IntelliJ. But this doesn't look like a valid java line. What is the default value of this variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method naming does not follow Java convention. Methods typically start with a lowercase letter, e.g. simpleTimeMarkFarPast().
The method name is not very descriptive. Consider something like getFarPastSimpleTimeMark() to clarify its intent.
If GenericWrapper and getIt() are needed, consider renaming getIt() to something more meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why is this wrapper needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a new thing, this has existed since June.
I'm fine with changing it if you really want me to but this is not a new thing. And the wrapper is needed because you can't assign a Kotlin class value to a Java object unless it's got a static definition, which
SimpleTimeMark.farPast()
is not. It's the same reason that right below that, we have a wrapper for Duration - they're composed types that simplify tolong
, making them easy to use in the storage, but initial values have to be wrapped.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use
new SimpleTimeMark(0)
instead then? really not a fan of this wrapping, and the function name breaks java conversations. so sad i have not noticed this beforeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we cannot.