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

Rubocop 3rd party templating #30

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Earlopain
Copy link
Contributor

@Earlopain Earlopain commented Dec 12, 2023

using rubocops 3rd party template support that got added in 1.45.

Oh, haven't seen this; awesome! Yeah, we should migrate to this new API.

Originally posted by @palkan in #28 (comment)


Not ready at all but I'll put this up now anyways so you can see what I'm up to. For now I have updated the preprocess tests to pass with the new api.

The really nice part of this is that you provide multiple code ranges, so there's no way that two codeblocks interact with eachother, like in #26.
A whole bunch of busywork is not needed anymore, like keeping track of the config_store. The ProcessedSource has everything you need. In addition the rubocop cache should just work, though I don't know for certain. There's a test for that, so let's just see if that passes.

TODO:

  • Fix integration tests
  • Assert that offense range points to the correct location Better integration tests #31
  • Add test where code is indented before the codeblock backticks start
  • Fix Indented codeblocks throw off all future codeblocks #28, might just happen as a byproduct of this
  • Validate autocorrect handles different indentation levels
  • Add functionality to disable rubocop, like with the readme workaround. Doesn't work like that anymore since this doesn't concern itself with anything outside the codeblocks. Perhaps integrating it into the block language like ````ruby ruboco:disable all` works. Github renders that just fine. Implement cop disabling #36 (comment)
  • Update readme
  • Update default config exclusions
  • Check performance

Get rid of the imported rubocop assertions.
This use case is now different enough that these just don't make sense anymore
This was previously happending automatically, but since we
now create our own PorcessedSource this is necessary
@Earlopain
Copy link
Contributor Author

Earlopain commented Mar 9, 2024

I've actually spend some time on continuing this PR. Learned about some rubocop internals which made that process much easier.

All tests are passing but there are still some things I want to do. They aren't explicitly handled in tests currently but I'll change that (in hopefully fewer months than this took).

@palkan if you got any feedback, I think this is at a good point for that now. Alternatively, I can leave it as is currently, and work on the other things in a separate PR.

@ocvit ocvit mentioned this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indented codeblocks throw off all future codeblocks
1 participant