[refurb] Implement write-whole-file (FURB103)#10802
Merged
dhruvmanila merged 6 commits intoastral-sh:mainfrom Apr 11, 2024
Merged
[refurb] Implement write-whole-file (FURB103)#10802dhruvmanila merged 6 commits intoastral-sh:mainfrom
refurb] Implement write-whole-file (FURB103)#10802dhruvmanila merged 6 commits intoastral-sh:mainfrom
Conversation
774638f to
d0cd4ae
Compare
augustelalande
commented
Apr 6, 2024
| 51 | # writes a single time to file and that bit they can replace. | ||
| | | ||
|
|
||
| FURB103.py:58:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(newline="\r\n", foobar)` |
Contributor
Author
There was a problem hiding this comment.
I understand that foobar is being placed after newline="\r\n" because the generator uses source order, and foobar comes later, but I'm not sure how to remedy it.
Member
There was a problem hiding this comment.
Yeah, that's unfortunate. I think I'd suggest to have two different variants of make_suggestion. We can keep the one in read_whole_file as is.
Here, one thing to note is that the write function only takes in one positional argument which we can assume. So, the make_suggestion in write_whole_file would take in the argument expression as an additional parameter.
We can use relocate_expr to relocate it to the default range before calling the make_suggestion function.
pub(super) fn make_suggestion(
open: &FileOpen<'_>,
arg: &Expr,
generator: Generator,
) -> SourceCodeSnippet {
let name = ast::ExprName {
id: open.mode.pathlib_method(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
let call = ast::ExprCall {
func: Box::new(name.into()),
arguments: ast::Arguments {
args: Box::new([arg.clone()]),
keywords: open.keywords.iter().copied().cloned().collect(),
range: TextRange::default(),
},
range: TextRange::default(),
};
SourceCodeSnippet::from_str(&generator.expr(&call.into()))
}
Contributor
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| FURB103 | 137 | 137 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
T-256
reviewed
Apr 9, 2024
| 21 | f.write(b"abc") | ||
| | | ||
|
|
||
| FURB103.py:24:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(encoding="utf8", foobar)` |
Contributor
There was a problem hiding this comment.
I think encoding="utf8" keyword argument should go after foobar positional argument
Contributor
Author
There was a problem hiding this comment.
Ya it's the same thing I commented above, but I'm not sure how to correct it.
dhruvmanila
requested changes
Apr 10, 2024
Member
dhruvmanila
left a comment
There was a problem hiding this comment.
Thanks you for the PR.
I think a lot of the definitions and functions are repeated as you've mentioned. It would be useful to move the common structs and functions to crates/ruff_linter/src/rules/refurb/helpers.rs module to avoid having duplicate logic. You can find similar examples in other rule modules as well. Can you please make this change?
71accbc to
2d35452
Compare
Contributor
Author
|
@dhruvmanila Done. Any suggestion on the comments above? |
2d35452 to
bc059af
Compare
dhruvmanila
requested changes
Apr 11, 2024
Member
dhruvmanila
left a comment
There was a problem hiding this comment.
Thank you for quickly following up with the requested change.
I've a few suggestion to avoid cloning and have provided a suggestion for the incorrect argument order that you've highlighted.
I haven't looked at the test cases yet which I'll do so now.
| 51 | # writes a single time to file and that bit they can replace. | ||
| | | ||
|
|
||
| FURB103.py:58:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(newline="\r\n", foobar)` |
Member
There was a problem hiding this comment.
Yeah, that's unfortunate. I think I'd suggest to have two different variants of make_suggestion. We can keep the one in read_whole_file as is.
Here, one thing to note is that the write function only takes in one positional argument which we can assume. So, the make_suggestion in write_whole_file would take in the argument expression as an additional parameter.
We can use relocate_expr to relocate it to the default range before calling the make_suggestion function.
pub(super) fn make_suggestion(
open: &FileOpen<'_>,
arg: &Expr,
generator: Generator,
) -> SourceCodeSnippet {
let name = ast::ExprName {
id: open.mode.pathlib_method(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
let call = ast::ExprCall {
func: Box::new(name.into()),
arguments: ast::Arguments {
args: Box::new([arg.clone()]),
keywords: open.keywords.iter().copied().cloned().collect(),
range: TextRange::default(),
},
range: TextRange::default(),
};
SourceCodeSnippet::from_str(&generator.expr(&call.into()))
}
Member
|
@augustelalande Sorry, I was playing around with the suggestions I made which fixes the "suggestion" problem, so I've pushed that. I'll list down what all changes I made, feel free to revert any of them if you think otherwise:
|
dhruvmanila
approved these changes
Apr 11, 2024
Member
dhruvmanila
left a comment
There was a problem hiding this comment.
Thank you for working on this!
Member
|
(Looking at the ecosystem checks, making sure they're correct.) |
Contributor
Author
|
Thanks for cleaning things up. |
Glyphack
pushed a commit
to Glyphack/ruff
that referenced
this pull request
Apr 12, 2024
## Summary Implement `write-whole-file` (`FURB103`), part of astral-sh#1348. This is largely a copy and paste of `read-whole-file` astral-sh#7682. ## Test Plan Text fixture added. --------- Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement
write-whole-file(FURB103), part of #1348. This is largely a copy and paste ofread-whole-file#7682.Test Plan
Text fixture added.