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

Replacing enums with string and installing isort & ruff #228

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

smttsp
Copy link
Contributor

@smttsp smttsp commented Oct 5, 2023

Summary

What is the reason for this Pull-Request?

This PR replaces HashType and IssueType enums with strings. IssueType was created before but in some places, string was used. This PR resolves that issue.

Along with those, ruff and isort which are linting and import formatting tools, respectively, added to in requirements.

If isort and ruff are not desired, I can remove them from the commits

NOTE: This PR will be merged after https://github.com/cleanlab/cleanvision/pull/227/files if it is merged. Then, we can get rid of the commits from that PR (by rebasing onto main). Currently, we see those changes here which will be gone after the rebasing.

Impact

What is the scope of your changes and who/where do you think they will affect?

Small code clean up and some formatting. Along with that additional installations. Normally, these installations should not go into production code, they should stay in dev but I am not sure how to do that in the current setting. In the future, by separating requirements from requirements-dev.txt, we may be able to achieve that.

Testing

What testing have you done and verified? Please link to the one unit test that is most end-to-end check of your new functionality. It's ok if your unit tests are not yet comprehensive when you first open the PR, we can revisit them later!

  • to be done

Ticket(s) or Conversations

What Git or Slack items (Issues, threads etc) are specifically related to this work? Please link them here.

References

Include any extra information that may be helpful to reviewers of your Pull-Request here (e.g. relevant URLs or Wiki pages that could help give background information). If relevant, please include additional code snippets (and their outputs/return values) showing alternative ways to use your newly added methods.

@smttsp smttsp changed the title Replacing enums with string and installing isort & ruff DRAFT PR: Replacing enums with string and installing isort & ruff Oct 5, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (cd8f98b) 95.63% compared to head (c2037fe) 95.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
+ Coverage   95.63%   95.68%   +0.04%     
==========================================
  Files          16       17       +1     
  Lines         986      997      +11     
  Branches      194      194              
==========================================
+ Hits          943      954      +11     
  Misses         22       22              
  Partials       21       21              
Files Coverage Δ
src/cleanvision/__init__.py 100.00% <ø> (ø)
src/cleanvision/dataset/base_dataset.py 100.00% <ø> (ø)
src/cleanvision/dataset/fsspec_dataset.py 95.65% <100.00%> (ø)
src/cleanvision/dataset/hf_dataset.py 100.00% <100.00%> (ø)
src/cleanvision/dataset/torch_dataset.py 100.00% <ø> (ø)
src/cleanvision/dataset/utils.py 100.00% <ø> (ø)
src/cleanvision/imagelab.py 91.97% <100.00%> (+0.04%) ⬆️
src/cleanvision/issue_managers/__init__.py 88.00% <100.00%> (-3.67%) ⬇️
...anvision/issue_managers/duplicate_issue_manager.py 99.21% <100.00%> (+<0.01%) ⬆️
src/cleanvision/issue_managers/image_property.py 94.00% <100.00%> (ø)
... and 7 more

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

@smttsp smttsp changed the title DRAFT PR: Replacing enums with string and installing isort & ruff Replacing enums with string and installing isort & ruff Oct 11, 2023
@smttsp
Copy link
Contributor Author

smttsp commented Oct 11, 2023

hey @jwmueller @sanjanag @elisno,

This PR is mostly related to formatting.
I eliminated half of the long lines, did some formatting, and created a hashing-type enum. Moved the issue type enum to another file as it was causing circular dependency.

I added the following libs to the requirements.

isort
loguru
ruff

I was thinking of replacing prints with logging sometime soon but not sure of your preferences. isort and ruff are formatting/linting tools that I have been using. I thought it would help. I used ruff for splitting long lines.

If you guys think this PR is an improvement, we can merge this branch after my other PR. Otherwise, I could drop the parts you think unnecessary.

@smttsp smttsp marked this pull request as ready for review October 11, 2023 00:11
pyproject.toml Show resolved Hide resolved
requirements-dev.txt Outdated Show resolved Hide resolved
Co-authored-by: Elías Snorrason <eliassno@gmail.com>
Copy link
Contributor Author

@smttsp smttsp left a comment

Choose a reason for hiding this comment

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

one comment

src/cleanvision/imagelab.py Show resolved Hide resolved
@smttsp
Copy link
Contributor Author

smttsp commented Oct 11, 2023

@elisno, actually, I was in the wrong branch when I tried running black after setting the line length to 100: #228 (comment). (you can ignore my previous message)

Previously, you guys didn't have black settings in the pyproject.toml. So you were using black with the default settings. By default, the line length is set to 88: https://safjan.com/black-change-max-line-length/#:~:text=Black%20is%20an%20opinionated%20code,by%20default%20to%2088%20characters.

So, setting it to 100 caused quite a bit of change in 19 files in cleanvision. Below is an example screenshot from my gitkraken after the change & running black with length=100:

Screenshot 2023-10-11 at 9 15 15 AM

Please let me know if you want me to commit the above changes. Or I can commit them and you can decide whether to keep or drop those commits. What do you think?

@smttsp smttsp requested a review from elisno October 11, 2023 13:20
@elisno
Copy link
Member

elisno commented Oct 11, 2023

Previously, you guys didn't have black settings in the pyproject.toml. So you were using black with the default settings. By default, the line length is set to 88: https://safjan.com/black-change-max-line-length/#:~:text=Black%20is%20an%20opinionated%20code,by%20default%20to%2088%20characters.

I see, then the 88-character limit you've set is fine for this PR. I just assumed that some of the formatting changes were caused by explicitly setting that character limit.

No need to bump it up to 100 for now.

requirements-dev.txt Outdated Show resolved Hide resolved
Co-authored-by: Elías Snorrason <eliassno@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants