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

[refurb] Implement write-whole-file (FURB103) #10802

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

augustelalande
Copy link
Contributor

Summary

Implement write-whole-file (FURB103), part of #1348. This is largely a copy and paste of read-whole-file #7682.

Test Plan

Text fixture added.

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)`
Copy link
Contributor Author

@augustelalande augustelalande Apr 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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()))
}

Copy link
Contributor

github-actions bot commented Apr 6, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+137 -0 violations, +0 -0 fixes in 5 projects; 39 projects unchanged)

apache/airflow (+35 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/cli/commands/kubernetes_command.py:70:14: FURB103 `open` and `write` should be replaced by `Path(...).write_text(yaml.dump(sanitized_pod))`
+ airflow/cli/commands/pool_command.py:147:10: FURB103 `open` and `write` should be replaced by `Path(filepath)....`
+ airflow/example_dags/example_kubernetes_executor.py:91:18: FURB103 `open` and `write` should be replaced by `Path("/foo/volume_mount_test.txt").write_text("Hello")`
+ airflow/providers/databricks/operators/databricks_sql.py:152:18: FURB103 `open` and `write` should be replaced by `Path(self._output_path)....`
+ airflow/providers/fab/auth_manager/security_manager/override.py:776:18: FURB103 `open` and `write` should be replaced by `Path(password_path).write_text(password)`
+ airflow/providers/google/cloud/hooks/cloud_sql.py:529:14: FURB103 `open` and `write` should be replaced by `Path(proxy_path_tmp).write_bytes(response.content)`
+ airflow/providers/imap/hooks/imap.py:291:14: FURB103 `open` and `write` should be replaced by `Path(file_path).write_bytes(payload)`
+ airflow/providers/microsoft/azure/hooks/wasb.py:391:14: FURB103 `open` and `write` should be replaced by `Path(file_path).write_bytes(stream.readall())`
+ dev/breeze/src/airflow_breeze/utils/docs_publisher.py:104:18: FURB103 `open` and `write` should be replaced by `Path(os.path.join(output_dir, "..", "stable.txt")).write_text(self._current_version)`
+ docs/exts/docs_build/dev_index_generator.py:68:10: FURB103 `open` and `write` should be replaced by `Path(out_file).write_text(content)`
+ docs/exts/redirects.py:80:18: FURB103 `open` and `write` should be replaced by `Path(redirected_filename).write_text(TEMPLATE.format(to_path))`
+ docs/exts/sphinx_script_update.py:87:10: FURB103 `open` and `write` should be replaced by `Path(cache_filepath).write_bytes(res.content)`
... 23 additional changes omitted for project

bokeh/bokeh (+41 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ examples/models/basic_plot.py:42:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/buttons.py:75:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Button widgets", doc))`
+ examples/models/calendars.py:104:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Calendar 2014", doc))`
+ examples/models/colors.py:69:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/custom.py:117:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/customjs.py:38:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/dateaxis.py:36:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/daylight.py:107:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Daylight Plot", doc))`
+ examples/models/gauges.py:110:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Gauges", doc))`
+ examples/models/glyphs.py:115:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="Glyphs", doc))`
+ examples/models/maps.py:59:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/maps_cities.py:46:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ examples/models/sliders.py:74:10: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(file_html(title="sliders", doc))`
+ examples/models/tile_source.py:38:10: FURB103 `open` and `write` should be replaced by `Path(filename)....`
... 27 additional changes omitted for project

rotki/rotki (+19 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ package.py:734:14: FURB103 `open` and `write` should be replaced by `Path(p12).write_bytes(certificate_data)`
+ rotkehlchen/chain/ethereum/airdrops.py:248:18: FURB103 `open` and `write` should be replaced by `Path(filename).write_text(response.text, encoding='utf8')`
+ rotkehlchen/chain/ethereum/airdrops.py:284:14: FURB103 `open` and `write` should be replaced by `Path(filename)....`
+ rotkehlchen/chain/ethereum/utils.py:224:10: FURB103 `open` and `write` should be replaced by `Path(avatars_dir / f'{ens_name}.png').write_bytes(avatar)`
+ rotkehlchen/db/dbhandler.py:300:14: FURB103 `open` and `write` should be replaced by `Path(self.user_data_dir / DBINFO_FILENAME).write_text(rlk_jsondumps(dbinfo), encoding='utf8')`
+ rotkehlchen/db/dbhandler.py:593:18: FURB103 `open` and `write` should be replaced by `Path(tempdbpath).write_bytes(unencrypted_db_data)`
+ rotkehlchen/icons.py:190:14: FURB103 `open` and `write` should be replaced by `Path(self.iconfile_path(asset)).write_bytes(response.content)`
+ rotkehlchen/tests/api/test_caching.py:42:10: FURB103 `open` and `write` should be replaced by `Path(f'{icons_dir}/ETH_small.png').write_bytes(b'')`
+ rotkehlchen/tests/api/test_caching.py:45:10: FURB103 `open` and `write` should be replaced by `Path(f'{icons_dir}/BTC_small.png').write_bytes(b'')`
+ rotkehlchen/tests/api/test_caching.py:48:10: FURB103 `open` and `write` should be replaced by `Path(f'{icons_dir}/AVAX_small.png').write_bytes(b'')`
... 9 additional changes omitted for project

zulip/zulip (+35 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ analytics/management/commands/check_analytics_state.py:37:14: FURB103 `open` and `write` should be replaced by `Path(state_file_tmp)....`
+ corporate/tests/test_stripe.py:169:18: FURB103 `open` and `write` should be replaced by `Path(fixture_path)....`
+ corporate/tests/test_stripe.py:298:14: FURB103 `open` and `write` should be replaced by `Path(fixture_file).write_text(file_content)`
+ scripts/lib/setup_venv.py:272:10: FURB103 `open` and `write` should be replaced by `Path(script_path).write_text("".join(lines))`
+ scripts/lib/zulip_tools.py:535:10: FURB103 `open` and `write` should be replaced by `Path(hash_path).write_text(new_hash)`
+ tools/lib/pretty_print.py:160:18: FURB103 `open` and `write` should be replaced by `Path(fn).write_text(phtml)`
+ tools/lib/provision.py:388:14: FURB103 `open` and `write` should be replaced by `Path(apt_hash_file_path).write_text(new_apt_dependencies_hash)`
+ tools/lib/provision_inner.py:366:10: FURB103 `open` and `write` should be replaced by `Path(version_file)....`
+ tools/setup/generate_integration_bots_avatars.py:54:10: FURB103 `open` and `write` should be replaced by `Path(bot_avatar_path).write_bytes(avatar)`
+ zerver/data_import/gitter.py:396:10: FURB103 `open` and `write` should be replaced by `Path(output_file)....`
+ zerver/data_import/import_util.py:749:10: FURB103 `open` and `write` should be replaced by `Path(output_file)....`
+ zerver/data_import/rocketchat.py:301:14: FURB103 `open` and `write` should be replaced by `Path(target_path).write_bytes(emoji_data)`
... 23 additional changes omitted for project

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ bin/maintenance/generate_moment_locales.py:100:10: FURB103 `open` and `write` should be replaced by `Path(output).write_text(header + locales)`
+ indico/cli/i18n.py:296:18: FURB103 `open` and `write` should be replaced by `Path(json_file).write_text(output)`
+ indico/cli/i18n.py:321:10: FURB103 `open` and `write` should be replaced by `Path(_get_messages_react_pot(directory)).write_bytes(output)`
+ indico/cli/setup.py:674:14: FURB103 `open` and `write` should be replaced by `Path(self.config_path).write_text(config + '\n')`
+ indico/web/assets/blueprint.py:111:14: FURB103 `open` and `write` should be replaced by `Path(cache_file)....`
+ indico/web/assets/blueprint.py:63:14: FURB103 `open` and `write` should be replaced by `Path(cache_file).write_text(data)`
+ setup.py:42:18: FURB103 `open` and `write` should be replaced by `Path(json_file).write_bytes(output)`

Changes by rule (1 rules affected)

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.

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)`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think encoding="utf8" keyword argument should go after foobar positional argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya it's the same thing I commented above, but I'm not sure how to correct it.

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

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?

@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Apr 10, 2024
@augustelalande augustelalande force-pushed the write-whole-file branch 3 times, most recently from 71accbc to 2d35452 Compare April 10, 2024 21:16
@augustelalande
Copy link
Contributor Author

@dhruvmanila Done. Any suggestion on the comments above?

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

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.

crates/ruff_linter/src/rules/refurb/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/refurb/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/refurb/helpers.rs Outdated Show resolved Hide resolved
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)`
Copy link
Member

Choose a reason for hiding this comment

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

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()))
}

@dhruvmanila
Copy link
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:

  1. Use copy semantics on OpenMode
  2. Make everything in helpers scoped to super
  3. Implement make_suggestion twice where the write_* one would take in a single argument, relocate it to the default text range and send it to the generator. This fixes the bug you commented about
  4. Avoid having multiple clones for the WriteMatcher struct

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

@dhruvmanila dhruvmanila added the preview Related to preview mode features label Apr 11, 2024
@dhruvmanila
Copy link
Member

(Looking at the ecosystem checks, making sure they're correct.)

@dhruvmanila dhruvmanila merged commit ffea1bb into astral-sh:main Apr 11, 2024
17 checks passed
@augustelalande
Copy link
Contributor Author

Thanks for cleaning things up. relocate_expr is exactly what I was looking for.

@augustelalande augustelalande deleted the write-whole-file branch April 11, 2024 18:35
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants