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

Added black trailing comma style fix #832

Merged
merged 10 commits into from
Mar 26, 2024

Conversation

VukW
Copy link
Contributor

@VukW VukW commented Mar 23, 2024

Fixes #830 : applies new black settings to the whole repo. No logic updates, linting style only

Proposed Changes

  • Resolve black CI versions conflict: now two black checks are executed. The first by psf/black@stable now runs the latest black version; the second, manual by python -m black --check . runs a hardcoded 23.11.0 version. As their required fixes differ, CI fails (it's not possible to fit both versions simultaneously). Thus , hardcoded 23.11.0 for first step also.
  • Fixed black trainling-comma setting
  • Changed black coverage behavior: instead of covering gandlf_* scripts only, black covers (almost) all the repo now
  • Excluded some files from black coverage: files that require style fixes not connected to the trailing-comma setting
  • Added all changes trailing-comma requires

Checklist

  • CONTRIBUTING guide has been followed.
  • PR is based on the current GaNDLF master .
  • Non-breaking change (does not break existing functionality): provide as many details as possible for any breaking change.
  • Function/class source code documentation added/updated (ensure typing is used to provide type hints, including and not limited to using Optional if a variable has a pre-defined value).
  • Code has been blacked for style consistency and linting.
  • If applicable, version information has been updated in GANDLF/version.py.
  • If adding a git submodule, add to list of exceptions for black styling in pyproject.toml file.
  • Usage documentation has been updated, if appropriate.
  • Tests added or modified to cover the changes; if coverage is reduced, please give explanation.
  • If customized dependency installation is required (i.e., a separate pip install step is needed for PR to be functional), please ensure it is reflected in all the files that control the CI, namely: python-test.yml, and all docker files [1,2,3].

Copy link
Contributor

github-actions bot commented Mar 23, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@VukW VukW changed the title Added black-comma style fix Added black trailing comma style fix Mar 23, 2024
Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.01%. Comparing base (3e0480a) to head (03b7ce9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
- Coverage   95.03%   95.01%   -0.02%     
==========================================
  Files         120      120              
  Lines        8270     8270              
==========================================
- Hits         7859     7858       -1     
- Misses        411      412       +1     
Flag Coverage Δ
unittests 95.01% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VukW VukW mentioned this pull request Mar 23, 2024
10 tasks
pyproject.toml Outdated Show resolved Hide resolved
.github/workflows/black.yml Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
Copy link
Collaborator

@sarthakpati sarthakpati left a comment

Choose a reason for hiding this comment

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

Added a small comment (needs to be checked for lint)

Co-authored-by: Sarthak Pati <patis@iu.edu>
@Geeks-Sid
Copy link
Collaborator

Is this trailing automated with a particular fixed version?

@sarthakpati
Copy link
Collaborator

Is this trailing automated with a particular fixed version?

Do you mean for the parameter skip_magic_trailing_comma? I think it should be supported in all versions since it was introduced. The idea to have the fixed version is to ensure that (if) any behavior changes are introduced by black in future versions, we know which version has been working for us, and making a change to incorporate any (hypothetical) future behavior is a deliberate decision on our part.

@VukW
Copy link
Contributor Author

VukW commented Mar 25, 2024

Is this trailing automated with a particular fixed version?

Sorry, @Geeks-Sid didn't get your question. The original goal was just to add skip_magic_trailing_comma = true setting into black config. However, during this fix a few additional issues arised:

  • Black didn't really check the repo code because of include = 'gandlf_*' setting in pyproject.toml. That made black to check only gandlf_* scripts, leaving all other files not covered. => Solution: remove this setting, covering all the project, adding all the necessary black changes.
  • In CI workflow black version in first step wasn't fixed, thus black CI always fails because checks are made by two conflicting black versions simultaneously => Solution: to fix black version in CI to 23.11.0 (just because we are using this version right now)

Not sure if I answered the question; if smth is still unclear, feel free to ask!

Copy link
Collaborator

@Geeks-Sid Geeks-Sid left a comment

Choose a reason for hiding this comment

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

Understood. LGTM. Quite long of a PR. Well done.

@Geeks-Sid Geeks-Sid merged commit c52de72 into mlcommons:master Mar 26, 2024
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2024
Copy link
Collaborator

@szmazurek szmazurek left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

4 participants