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

[ci] [python-package] enable ruff-format on all Python code #6336

Merged
merged 19 commits into from
Feb 27, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Feb 21, 2024

Contributes to #6304.

Proposes enforcing ruff-format on all of the Python code checked into source control in this project.

This only has changes in python-package/ because previous PRs touched other files:

Notes for reviewers

I made a few manual changes. I've added comments on the relevant parts of the diff explaining those.

After this, I'll put up one more PR with a .git-blame-ignore-revs file, dropping all these large reformatting changes from the git blame.

Relevant docs:

return self

def __init_from_csc(
self,
csc: scipy.sparse.csc_matrix,
params_str: str,
ref_dataset: Optional[_DatasetHandle]
ref_dataset: Optional[_DatasetHandle],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Commenting on this part of the diff to share a general point.... I added many of the trailing commas in this PR manually.

Without that trailing command, ruff / black wants to turn this:

def __init_from_csc(
    self,
    csc: scipy.sparse.csc_matrix,
    params_str: str,
    ref_dataset: Optional[_DatasetHandle]
) -> "Dataset"

Into this:

def __init_from_csc(
    self, csc: scipy.sparse.csc_matrix, params_str: str, ref_dataset: Optional[_DatasetHandle]
) -> "Dataset"

For details on that, see https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma.

And this:

skip-magic-trailing-comma = false

@@ -2617,7 +2730,7 @@ def _reverse_update_params(self) -> "Dataset":
def set_field(
self,
field_name: str,
data: Optional[Union[List[List[float]], List[List[int]], List[float], List[int], np.ndarray, pd_Series, pd_DataFrame, pa_Table, pa_Array, pa_ChunkedArray]]
data: Optional[_LGBM_SetFieldType],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I manually pulled this out into a new type, LGBM_SetFieldType, because ruff wanted to split this into a one-type-per-line list, which I thought made the function signature difficult to read.

first_metric_only: bool = False,
verbose: bool = True,
min_delta: Union[float, List[float]] = 0.0,
) -> _EarlyStoppingCallback:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this one manually, for consistency with all the other has-more-than-1-argument functions.

self,
preds: np.ndarray,
dataset: Dataset,
) -> Tuple[np.ndarray, np.ndarray]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I accidentally made this change manually, while trying to revert ruff's change to the other __call__() call a bit further down... but I think we should keep this change. That accident just revealed that these were inconsistent with each other, and I like the one {argument}: {type} per line style.

@jameslamb jameslamb changed the title WIP: [ci] [python-package] enable ruff-format on all Python code [ci] [python-package] enable ruff-format on all Python code Feb 22, 2024
@jameslamb jameslamb marked this pull request as ready for review February 22, 2024 03:36
@jameslamb
Copy link
Collaborator Author

Thanks @guolinke ! I'd really like @jmoralez 's opinion on this one as well before we merge.

Copy link
Collaborator

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

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

Thanks!

@jameslamb jameslamb merged commit dd31208 into master Feb 27, 2024
43 checks passed
@jameslamb jameslamb deleted the ci/even-more-formatting branch February 27, 2024 16:53
@jameslamb
Copy link
Collaborator Author

Thanks so much @jmoralez @borchero @guolinke !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants