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

[Fix #184] Fix Node#parent_module_name for sclass nodes #197

Merged
merged 1 commit into from Aug 10, 2021

Conversation

dvandersluis
Copy link
Member

Continuation of #177. I took @marcandre's tests and got them to pass. This fixes rubocop/rubocop#5022, but rubocop itself needs a PR as well because this change adds elements to the parent_module_name that it is not currently expecting.

@dvandersluis
Copy link
Member Author

There's a bit of a catch 22 between this PR and rubocop/rubocop#9994. Each PR needs the other to get the tests passing.

@marcandre
Copy link
Contributor

There's a bit of a catch 22 between this PR and rubocop/rubocop#9994. Each PR needs the other to get the tests passing.

Right. So either we merge this and reset the CI suite to test from the next version of RuboCop, or else we introduce some flag.

Let me see how your additional tests fail with the previous code

@marcandre
Copy link
Contributor

Yeah, no flag.

So, short of introducing a list of tests that are ok to fail, I think the only way forward is:

  1. merge this
  2. tweak the CI suite by removing tests on RuboCop 0.92
  3. release a bug fix release
    On RuboCop's side:
  4. merge PR
  5. bump the minimum requirement for rubocop-ast
  6. release a version of RuboCop
    Back on RuboCop::AST:
  7. tweak the CI suite to restore testing on that version of RuboCop

All RuboCop PRs between step 3 and 4 will have their CI fail though.

@dvandersluis do you see a better way?

@dvandersluis
Copy link
Member Author

dvandersluis commented Aug 10, 2021

So like you said the problem is that this change will break existing tests in rubocop, even without the new tests, because the method return is going to be different.

I'm good with removing rubocop 0.92 from the test suite, since I'm not really sure why we're running against a really old rubocop anyways? However, if we need it we can always add it back when done (or maybe a more recent version?).

I think maybe a less disruptive path would be:

If we agree with this (and are good with both this and the rubocop PR, obviously) I can do all this quickly and sequentially for minimal disruption (the only thing I don't know how to do would be to release a new version of the gem).

@marcandre
Copy link
Contributor

we're running against a really old rubocop anyways

Ideally, projects with old versions of RuboCop (e.g. stuck with old versions of Ruby) can still run with newer rubocop-ast.

If we agree with this

Indeed, marking the tests as pending is the way to go 👍 Let's do this.

I'll be glad to make the release. If you create the PR to tweak the CI suite, don't forget to modify the .mergify too

@dvandersluis
Copy link
Member Author

@marcandre once #198 is merged, I'll rebase and merge this PR.

@dvandersluis
Copy link
Member Author

@marcandre this is ready to be merged and released now.

@marcandre
Copy link
Contributor

Released 1.9.1, thanks! 👍

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.

Lint/DuplicateMethods for classes inside class << self of parent module
2 participants