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

Fix type hints #150

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Fix type hints #150

merged 3 commits into from
Oct 12, 2023

Conversation

Mulugruntz
Copy link
Contributor

@Mulugruntz Mulugruntz commented Oct 11, 2023

This fixes some type hint issues that are preventing downstream projects from passing CI.

This contains the following changes:

  • SizeLimitedFile uses common conventions, types and defaults.
  • _fileobj_name -> _obj_name, as it's not related to files.
  • rohmufile.write_file only requires HasRead for the input file.
  • HasRead, HasWrite use Python conventions, types and defaults.
  • FileLike uses Python conventions, types and defaults.
  • New HasName.

Why this way

Signed-off-by: Samuel Giffard <[email protected]>
@Mulugruntz Mulugruntz self-assigned this Oct 11, 2023
@Mulugruntz Mulugruntz marked this pull request as draft October 11, 2023 15:26
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-2272-fix-type-hints branch from 58f1fc3 to f595f12 Compare October 11, 2023 15:35
Samuel Giffard added 2 commits October 11, 2023 17:35
This contains the following changes:

- `SizeLimitedFile` uses common conventions, types and defaults.
- `_fileobj_name` -> `_obj_name`, as it's not related to files.
- `rohmufile.write_file` only requires `HasRead` for the input file.
- `HasRead`, `HasWrite` use Python conventions, types and defaults.
- `FileLike` uses Python conventions, types and defaults.
- New `HasName`.

Signed-off-by: Samuel Giffard <[email protected]>
@Mulugruntz Mulugruntz force-pushed the sgiffard/BF-2272-fix-type-hints branch from f595f12 to 0931939 Compare October 11, 2023 15:35
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

Merging #150 (0931939) into main (a37601a) will increase coverage by 0.01%.
The diff coverage is 65.11%.

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   66.76%   66.77%   +0.01%     
==========================================
  Files          35       35              
  Lines        3899     3904       +5     
==========================================
+ Hits         2603     2607       +4     
- Misses       1296     1297       +1     
Files Coverage Δ
rohmu/encryptor.py 81.45% <100.00%> (ø)
rohmu/factory.py 92.85% <100.00%> (ø)
rohmu/filewrap.py 83.33% <100.00%> (ø)
rohmu/snappyfile.py 82.75% <100.00%> (ø)
rohmu/util.py 74.07% <100.00%> (ø)
rohmu/zstdfile.py 29.50% <100.00%> (ø)
rohmu/atomic_opener.py 90.00% <0.00%> (ø)
rohmu/object_storage/s3.py 65.15% <0.00%> (ø)
rohmu/object_storage/sftp.py 47.22% <0.00%> (ø)
rohmu/rohmufile.py 36.28% <75.00%> (ø)
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

fingon
fingon previously requested changes Oct 12, 2023
Copy link
Contributor

@fingon fingon left a comment

Choose a reason for hiding this comment

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

If these whitespace changes are required by other projects, there should be autoformat step to ensure future rohmu changes also fullfill these requirements. (And ideally also provides autofixed formatting if available.)

@Mulugruntz
Copy link
Contributor Author

Mulugruntz commented Oct 12, 2023

If these whitespace changes are required by other projects, there should be autoformat step to ensure future rohmu changes also fullfill these requirements. (And ideally also provides autofixed formatting if available.)

You're right, an autoformat would be great.

They are not required for the current problem this PR is fixing. If you like I can drop this particular commit that removes the spaces.

To be noted that the standard is without the space and that many tools will show a lot of alerts when that space is there.
https://mypy.readthedocs.io/en/stable/common_issues.html#spurious-errors-and-locally-silencing-the-checker

You can use the form # type: ignore[<code>] to only ignore specific errors on the line.

@Mulugruntz Mulugruntz dismissed fingon’s stale review October 12, 2023 11:00

Please see my comment, and tell me if you wish me to remove this "ignore" commit.

Copy link
Contributor

@fingon fingon left a comment

Choose a reason for hiding this comment

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

It is fine to include it too. I don't mind the change, I just want to avoid hidden dependencies on stricter validation in other projects.

@fingon fingon merged commit 61cef02 into main Oct 12, 2023
@fingon fingon deleted the sgiffard/BF-2272-fix-type-hints branch October 12, 2023 11:02
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.

3 participants