[refurb] Extend support for Path.open (FURB101, FURB103)

[11happy]

Summary

This PR fixes #18409

Test Plan

I have added tests in FURB103.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+15 -0 violations, +0 -0 fixes in 4 projects; 51 projects unchanged)

PlasmaPy/PlasmaPy (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ .github/scripts/authors_in_cff.py:40:10: FURB101 `Path.open()` followed by `read()` can be replaced by `pathlib.Path("CITATION.cff").read_text()`
+ .github/scripts/citation_updater.py:69:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `citation_rst_file.write_text(citation_rst_text)`
+ docs/_author_list_from_cff.py:193:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `pathlib.Path(rst_file).write_text(authors_rst, encoding="utf-8")`
+ docs/_global_substitutions.py:217:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `pathlib.Path(rst_file).write_text(content, encoding="utf-8")`
+ tests/utils/data/test_downloader.py:191:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `filepath.write_text("Not data")`

langchain-ai/langchain (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ libs/partners/chroma/langchain_chroma/vectorstores.py:489:14: FURB101 `Path.open()` followed by `read()` can be replaced by `Path(uri).read_bytes()`
+ libs/standard-tests/langchain_tests/conftest.py:70:14: FURB101 [*] `Path.open()` followed by `read()` can be replaced by `cassette_path.read_bytes()`
+ libs/standard-tests/langchain_tests/conftest.py:88:14: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `cassette_path.write_bytes(data)`

scikit-build/scikit-build-core (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ src/scikit_build_core/ast/ast.py:88:10: FURB101 `Path.open()` followed by `read()` can be replaced by `Path(sys.argv[1]).read_text(encoding="utf-8-sig")`
+ src/scikit_build_core/ast/tokenizer.py:77:10: FURB101 `Path.open()` followed by `read()` can be replaced by `Path(sys.argv[1]).read_text(encoding="utf-8-sig")`
+ src/scikit_build_core/metadata/regex.py:58:10: FURB101 `Path.open()` followed by `read()` can be replaced by `Path(input_filename).read_text(encoding="utf-8")`
+ tests/test_dynamic_metadata.py:274:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("__init__.py").write_text("__version__ = '0.1.0'")`
+ tests/test_dynamic_metadata.py:290:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("version.hpp")....`
+ tests/test_dynamic_metadata.py:326:10: FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("version.hpp")....`

indico/indico (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ indico/vendor/django_mail/message.py:369:14: FURB101 `Path.open()` followed by `read()` can be replaced by `path.read_bytes()`

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
FURB103	8	8	0	0	0
FURB101	7	7	0	0	0

[11happy]

@MichaReiser would appreciate your feedback on this whenever you get some time.

Thank you : )

[ntBre]

Thanks for working on this!

I think we need to be a bit more careful with how we identify existing Path instances and hopefully we can deduplicate some code too.

[amyreese]

It might be helpful to reuse is_open_call_from_pathlib from ASYNC230:

ruff/crates/ruff_linter/src/rules/flake8_async/rules/blocking_open_call.rs

Line 73 in 1c7ea69

fn is_open_call_from_pathlib(func: &Expr, semantic: &SemanticModel) -> bool {

[amyreese]

Alternately PathlibPathChecker which can also match annotations:

ruff/crates/ruff_linter/src/rules/flake8_async/rules/blocking_path_methods.rs

Line 161 in 1c7ea69

impl TypeChecker for PathlibPathChecker {

[11happy]

the common helper has too many arguments causing the clippy error, should I break it down ?

[ntBre]

the common helper has too many arguments causing the clippy error, should I break it down ?

I think it's okay just to add #[expect(clippy::too_many_arguments)] on the function.

[ntBre]

Are we missing some of the argument validation code from find_file_open, especially these lines:

    if args.iter().any(Expr::is_starred_expr)
        || keywords.iter().any(|keyword| keyword.arg.is_none())
    {
        return None;
    }

I also think we're doing too much inspection on the attr.value. For example, I think is_open_call_from_pathlib should be able to identify a case like this:

p = Path("file.txt")
with p.open("r") as f:
    f.write("test")

but the current implementation expects a literal Path() call, not a name that resolves to a Path.

We might need to make the filename field optional on the FileOpen struct.

[11happy]

done , also the message is formatted accordingly but i feel its getting nested & dosent feel good , not sure although

[11happy]

also I have not added any comments yet , should i add similar comments like find_file_open

[MichaReiser]

Would you mind rebasing your PR? Doing so will allow us to merge your PR without delayed once reviewed.

[11happy]

done rebased it & resolved merge conflicts : )
Thank you

[ntBre]

Thanks for rebasing and for your work on this! I pushed a commit fixing a few nits but then noticed a few more minor issues. The implementation feels a bit complicated to me now, I want to keep thinking about it.

[11happy]

Thanks for rebasing and for your work on this! I pushed a commit fixing a few nits but then noticed a few more minor issues. The implementation feels a bit complicated to me now, I want to keep thinking about it.

I agree with you implementation is complex ,felt the same, would appreciate any pointers to decomplicate/refactor it ?
Thank you : )

[ntBre]

I just pushed a commit trying out an OpenArgument enum since the path_obj field and filename field were basically mutually exclusive. I think this makes things a little simpler, but I'm curious to get your thoughts on it too. I think the only remaining thing I don't love is the is_path_open check on the Violation struct. Maybe we could use the whole suggestion we generate for the fix in the error message?

I also fixed some new merge conflicts while I was here.

I also noticed that we haven't updated the code for FURB102, even though it's quite similar to FURB103. Do you want to do that in a follow-up PR?

[11happy]

I just pushed a commit trying out an OpenArgument enum since the path_obj field and filename field were basically mutually exclusive. I think this makes things a little simpler, but I'm curious to get your thoughts on it too. I think the only remaining thing I don't love is the is_path_open check on the Violation struct. Maybe we could use the whole suggestion we generate for the fix in the error message?

I also fixed some new merge conflicts while I was here.

I also noticed that we haven't updated the code for FURB102, even though it's quite similar to FURB103. Do you want to do that in a follow-up PR?

Thank you for these updates, will go through the commit in detail & will share my thoughts, I am also interested to work on the follow up PR for FURB102 , I feel a little occupied this week but will update this PR & my other open PRs soon.
Thanks again : )

[11happy]

I just pushed a commit trying out an OpenArgument enum since the path_obj field and filename field were basically mutually exclusive. I think this makes things a little simpler, but I'm curious to get your thoughts on it too. I think the only remaining thing I don't love is the is_path_open check on the Violation struct. Maybe we could use the whole suggestion we generate for the fix in the error message?

Yes, with the new OpenArgument enum code is a lot cleaner, what do you think of using OpenArgument directly instead of is_path_open ?

[ntBre]

Thank you! The code for FURB103 looks good to me now, your last commit was a nice improvement :)

Based on the ecosystem report, this is already affecting FURB101 too (I had the wrong rule code when I mentioned FURB102 earlier). Would you mind going ahead and updating the FURB101 code and adding a test for it? I don't think it makes as much sense to save that for a follow-up if we already changed the rule's behavior here.

I think it's mostly just the message, fix_title, and filename_code that need to be updated, most of the logic is already in the shared helpers.

[11happy]

Thank you! The code for FURB103 looks good to me now, your last commit was a nice improvement :)

Based on the ecosystem report, this is already affecting FURB101 too (I had the wrong rule code when I mentioned FURB102 earlier). Would you mind going ahead and updating the FURB101 code and adding a test for it? I don't think it makes as much sense to save that for a follow-up if we already changed the rule's behavior here.

I think it's mostly just the message, fix_title, and filename_code that need to be updated, most of the logic is already in the shared helpers.

done, to keep it consistent with FURB103 separated the tests for FURB101, also should I add more tests ?
Thank you : )

[ntBre]

Thank you! I think this is enough testing for FURB101 since most of the tricky code is shared with FURB103.