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
Apply Black 2024 to codebase #5252
Conversation
@@ -102,30 +102,24 @@ def suggest_int( | |||
return self._trial.suggest_int(name, low, high, step=step, log=log) | |||
|
|||
@overload | |||
def suggest_categorical(self, name: str, choices: Sequence[None]) -> None: | |||
... |
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.
What about using pass
or raise NotImplemented
instead of ...
?
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.
I have no strong opinion on this topic but it is reasonable to ignore E704
in this case as mentioned in psf/black#3887 and PEP8
. I think it is possible to put pass
since NotImplementedError
raises when function with @overload
is actually called no matter what we write in the body .
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.
IMO, changing the implementation is out of scope of this PR, since this PR aims just to introduce black 2024.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5252 +/- ##
==========================================
+ Coverage 89.23% 89.43% +0.19%
==========================================
Files 206 206
Lines 12944 12914 -30
==========================================
- Hits 11550 11549 -1
+ Misses 1394 1365 -29 ☔ View full report in Codecov by Sentry. |
@contramundum53 Could you review this PR? |
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.
LGTM.
It would be great to add |
@HideakiImamura |
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.
I will approve this PR after #5252 (comment) is addressed.
@Alnusjaponica Could you resolve the conflict? I think lightgbm tuner has been moved to |
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.
Now, we can remove NOQA
from:
- optuna/trial/_base.py
- optun/trial/_fixed.py
- optuna/trial/_frozen.py
- optuna/trial_trial.py
Btw, were the merged changes also formatted already?
@nabenabe0928
It should be formatted since |
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.
Thank you for the changes, LGTM!
Motivation
Remove version specification on black introduced by #5210
Description of the changes
As all the issues mentioned in this comment resolved by
black 24.2.0
, I applied the new black formatting style to the codebase.Please note that 2024 black format for
@overload
is incompatible with flake8E704
, therefore the error message from flake8 is suppressed.