Skip to content

Comments

[red-knot] Enforce specifying paths for mdtest code blocks in a separate preceding line#15890

Merged
sharkdp merged 8 commits intoastral-sh:mainfrom
InSyncWithFoo:rk-mdtests-paths-2
Feb 4, 2025
Merged

[red-knot] Enforce specifying paths for mdtest code blocks in a separate preceding line#15890
sharkdp merged 8 commits intoastral-sh:mainfrom
InSyncWithFoo:rk-mdtests-paths-2

Conversation

@InSyncWithFoo
Copy link
Contributor

@InSyncWithFoo InSyncWithFoo commented Feb 2, 2025

Summary

Resolves #15695, rework of #15704.

This change modifies the Mdtests framework so that:

  • Paths must now be specified in a separate preceding line:

     `a.py`:
    
     ```py
     x = 1
     ```

    If the path of a file conflicts with its lang, an error will be thrown.

  • Configs are no longer accepted. The pattern still take them into account, however, to avoid "Unterminated code block" errors.

  • Unnamed files are now assigned unique, lang-respecting paths automatically.

Additionally, all legacy usages have been updated.

Test Plan

Unit tests and Markdown tests.

@InSyncWithFoo
Copy link
Contributor Author

Removing unnecessary paths is a tedious task and thus not tackled in this PR. If anyone wants to do so, please, be my guest.

Copy link
Contributor

@sharkdp sharkdp 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! I think we can improve the error message for developers a bit, but otherwise this looks great.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Other than the change to require two newlines before the path, this looks good to me. Thank you @InSyncWithFoo !

It looks like this doesn't implement @sharkdp 's suggestion from #15704 that consecutive unnamed code blocks would be merged into a single file. I like that idea, but I also think it's probably better if we leave that as a separate change/improvement, rather than adding it to this PR. We could file an issue to track the idea?

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for all the thorough tests!

It looks to me like the updates to README.md need a bit of work still, I think it'll be easiest if I just make some additional updates there and then merge.

There's also one code change I commented inline that I think I will revert, unless there's a reason for it that I'm not seeing.

@carljm
Copy link
Contributor

carljm commented Feb 3, 2025

@sharkdp I will hold off on merging this since you still have a change-request on it; go ahead and merge if it looks OK to you?

Copy link
Contributor

@sharkdp sharkdp 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 very much!

@sharkdp sharkdp merged commit 11cfe2e into astral-sh:main Feb 4, 2025
21 checks passed
dcreager added a commit that referenced this pull request Feb 4, 2025
* main: (66 commits)
  [red-knot] Use ternary decision diagrams (TDDs) for visibility constraints (#15861)
  [`pyupgrade`] Rename private type parameters in PEP 695 generics (`UP049`) (#15862)
  Simplify the `StringFlags` trait (#15944)
  [`flake8-pyi`] Make `PYI019` autofixable for `.py` files in preview mode as well as stubs (#15889)
  Docs (`linter.md`): clarify that Python files are always searched for in subdirectories (#15882)
  [`flake8-pyi`] Make PEP-695 functions with multiple type parameters fixable by PYI019 again (#15938)
  [red-knot] Use unambiguous invalid-syntax-construct for suppression comment test (#15933)
  Make `Binding::range()` point to the range of a type parameter's name, not the full type parameter (#15935)
  Update black deviations (#15928)
  [red-knot] MDTest: Fix line numbers in error messages (#15932)
  Preserve triple quotes and prefixes for strings (#15818)
  [red-knot] Hand-written MDTest parser (#15926)
  [`pylint`] Fix missing parens in unsafe fix for `unnecessary-dunder-call` (`PLC2801`) (#15762)
  nit: docs for ignore & select (#15883)
  [airflow] `BashOperator` has been moved to `airflow.providers.standard.operators.bash.BashOperator` (AIR302) (#15922)
  [`flake8-logging`] `.exception()` and `exc_info=` outside exception handlers (`LOG004`, `LOG014`) (#15799)
  [red-knot] Enforce specifying paths for mdtest code blocks in a separate preceding line (#15890)
  [red-knot] Internal refactoring of visibility constraints API (#15913)
  [red-knot] Implicit instance attributes (#15811)
  [`flake8-comprehensions`] Handle extraneous parentheses around list comprehension (`C403`) (#15877)
  ...
@InSyncWithFoo InSyncWithFoo deleted the rk-mdtests-paths-2 branch February 4, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to testing Ruff itself ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[red-knot] Explore alternative notation for declaring file names in mdtests

5 participants