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

Gemify Node & NodePattern #7735

Closed
marcandre opened this issue Feb 19, 2020 · 14 comments
Closed

Gemify Node & NodePattern #7735

marcandre opened this issue Feb 19, 2020 · 14 comments

Comments

@marcandre
Copy link
Contributor

I'd like to resurrect #6686 to isolate RuboCop::AST::* and RuboCop::NodePattern in its own separate gem.

I'm proposing:

  1. both RuboCop::AST::Node and RuboCop::NodePattern in the same gem
  2. short name: rubocop-ast
  3. no change in the code or hierarchy itself, only in which gem it is found. File paths would also remain the same.
  4. require 'rubocop-ast' would not do anything besides (auto-)loading the relevant classes.
  5. separate versioning from RuboCop's.
  6. recreate commit history in the rubocop-ast gem.

@jonatas has offered to help.

I've created a temporary repository to process the split. It has rubocop and rubocop-ast as two submodules.

I'd appreciate it very much if the powers that be let me know if there's anything they would like done differently.

@Drenmi
Copy link
Collaborator

Drenmi commented Feb 20, 2020

I think my biggest concern now might be how much overhead this adds in terms of fixing bugs related to the node patterns or node extensions in RuboCop Core and its extensions.

On another note, I think something like parser-tools or parser-extras might be nicer, as it's more descriptive, and there are many other projects eying this. 🙂

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 20, 2020

Our extension has little to do with code parsing and a lot more with the ast gem (that parser uses internally), that's why I like the name rubocop-ast much better. We can't really change any namespacing in the gem, as this is going to break everything depending on RuboCop, so having RuboCop in the name is fine - for me even though this gem is reusable it's also an integral part of RuboCop and it's going to continue driving its development.

I think my biggest concern now might be how much overhead this adds in terms of fixing bugs related to the node patterns or node extensions in RuboCop Core and its extensions.

Yep, that's true. The AST functionality is relatively stable these days, so we can probably live with the overhead, though.

@marcandre I'm fine with the points you've outlined.

@Drenmi
Copy link
Collaborator

Drenmi commented Feb 20, 2020

Yep, that's true. The AST functionality is relatively stable these days, so we can probably live with the overhead, though.

Actually, thinking a bit more about it, it might be easier when RuboCop Core and extensions all use this dependency, as they can upgrade separately.

We can't really change any namespacing in the gem, as this is going to break everything depending on RuboCop [...]

Good point. 👍

@marcandre
Copy link
Contributor Author

So I have managed to split rubocop/ast and node_pattern into a separate gem.

I have a reasonably easy process that can redo the split from scratch given a fresh rubocop repo, so whenever we are ready to proceed it is easy to do so.

It uses git-filter-repo to maintain all history concerning the relevant files, i.e. all commits that touched any of the ast/*, node_pattern, etc. are still there, with just the changes affecting those files: commits here

For developing purposes, if the two repositories rubocop and rubocop-ast are in the same folder, they will use each other, otherwise they will rely on gems. Rubocop repo after the split is here (last two commits would be squashed together)

What I can't do:

  • create rubocop-hq/rubocop-ast
  • configure the CI. Ideally the rubocop repo would be cloned next to rubocop-ast but it isn't strictly necessary.
  • register rubocop-ast gem

Let me know what else I can do

@bbatsov
Copy link
Collaborator

bbatsov commented May 12, 2020

@marcandre Thanks for working on this! I'll invite you now to our GitHub org, create the repo and register the gem name.

We're about to cut RuboCop 1.0 later this week, so it'd be nice if we dealt with the extraction before this.

@marcandre
Copy link
Contributor Author

Sounds great. Can't see the invite though.

@bbatsov
Copy link
Collaborator

bbatsov commented May 12, 2020

@marcandre You should see it now. The repo is also created. I'll register the gem name in a moment.

@bbatsov
Copy link
Collaborator

bbatsov commented May 12, 2020

Ah, actually it seems that I'll have to push the gem to register it, so I guess I'll do this after you import the code to the rubocop-ast repo.

@marcandre
Copy link
Contributor Author

Done

@bbatsov
Copy link
Collaborator

bbatsov commented May 12, 2020

Gem pushed. I guess we'll go pretty quickly from 0.0.1 to 1.0.0 on this one. :-)

@marcandre
Copy link
Contributor Author

Yeah, that's the plan. I'll see if I can setup the actions properly.

@marcandre
Copy link
Contributor Author

I would like to release rubocop-ast 0.0.2 but I don't have the access. Could you either add me gem owner rubocop-ast --add github@marc-andre.ca or release it please? Release commit & tag are pushed already.

marcandre added a commit to marcandre/rubocop that referenced this issue May 12, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented May 12, 2020

Could you either add me gem owner rubocop-ast --add github@marc-andre.ca

Done.

@marcandre
Copy link
Contributor Author

Awesome, thank you

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

No branches or pull requests

3 participants