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

Use SourceKind::diff for formatter #8240

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Use SourceKind::diff for formatter #8240

merged 1 commit into from
Oct 26, 2023

Conversation

dhruvmanila
Copy link
Member

Summary

This PR refactors the formatter diff code to reuse the SourceKind::diff logic. This has the benefit that the Notebook diff now includes the cell numbers which was not present before.

Test Plan

Update the snapshots and verified the cell numbers.

Comment on lines -157 to +162
_ => bail!("cannot diff Python source code with Jupyter notebook source code"),
_ => panic!("cannot diff Python source code with Jupyter notebook source code"),
Copy link
Member Author

Choose a reason for hiding this comment

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

We could potentially create a new SourceError::IncompatibleDiffSource variant but then the Diagnostics:from_source_error needs to be updated which I don't think is correct because the diff error shouldn't really be a diagnostic.

Copy link
Member

Choose a reason for hiding this comment

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

I really wish we had a way to enforce this statically through types (i.e., that this path should never be reached).

@dhruvmanila dhruvmanila requested a review from konstin October 26, 2023 03:48
@dhruvmanila dhruvmanila added the formatter Related to the formatter label Oct 26, 2023
@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+0, -0, 1 error(s))

build (error) https://github.com/pypa/build ref main select ignore exclude

ruff failed
  Cause: TOML parse error at line 2, column 19
  |
2 | requires = ['bad' 'syntax']
  |                   ^
invalid array
expected `]`


@dhruvmanila dhruvmanila merged commit a7d1f7e into main Oct 26, 2023
17 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/formatter-diff branch October 26, 2023 05:38
@konstin
Copy link
Member

konstin commented Oct 26, 2023

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants