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

Extract NodePattern to a separated gem #6686

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jonatas
Copy link
Contributor

jonatas commented Jan 18, 2019

I started the work related to #6655.

The gem extracted is under my namespace and I can move it soon some maintainer transfer it.

https://github.com/jonatas/rubocop-node_pattern

The name of the gem is also in the TODO list to be discussed and probably we can find a better name since all AST utilities are going to the repo too.

TODO

  • Move the related project to rubocop-hq namespace

  • Update the Gemfile to rubocop-hq namespace

  • Move NodePattern implementation to NodePattern::Parser or something to make the gem clear and avoid requires in the bottom

  • Define a better name to the gem?

  • Setup RuboCop config for the project

  • Wrote good commit messages.

  • Commit message starts with [Fix #issue-number] (if the related issue exists).

  • Feature branch is up-to-date with master (if not - rebase it).

  • Squashed related commits together.

  • Added tests.

  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.

  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Notes

I'm not sure how can we maintain the ownership of the current git files that are moving to another repo. Is it something common or we just move and have a new author?

@jonatas jonatas force-pushed the jonatas:extract-rubocop-node_pattern-gem branch from efc89ac to c865e01 Jan 18, 2019

@@ -14,6 +14,9 @@ gem 'simplecov', '~> 0.10'
gem 'test-queue'
gem 'yard', '~> 0.9'

gem 'rubocop-node_pattern',
git: 'git://github.com/jonatas/rubocop-node_pattern.git'

This comment has been minimized.

@jonatas

jonatas Jan 18, 2019

Author Contributor

Soon we migrate the gem to the proper repo, we can also publish as a gem and remove the git reference.

This comment has been minimized.

@wevtimoteo

wevtimoteo Jan 21, 2019

Did you consider keeping git history using filter-branch feature for these files?

https://help.github.com/articles/splitting-a-subfolder-out-into-a-new-repository/

This comment has been minimized.

@jonatas

jonatas Jan 22, 2019

Author Contributor

That is pretty cool @wevtimoteo, I'll use the tool to keep the git history intact 👍

This comment has been minimized.

@wevtimoteo

wevtimoteo Feb 8, 2019

If you need some help with that I'll be glad to pair with you!

Setup bundler to require node pattern
I couldn't find a way to make it happen without request bundler setup.

My guess is that `add_runtime_dependency` works differently and soon
we update the gem namespace and publish it, we can remove the
requirement from here.
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Jan 26, 2019

Keep in mind that the node extensions and the note pattern matcher are somewhat orthogonal - you don’t necessary always need the two of them. This means that potentially we can have an astx gem and some other gem about the pattern matcher. On the other hand this would mean more overhead from a maintenance perspective, so perhaps a single gem would do. I also think it might be convenient to simply have those 2 or 3 gems in the same repo (RuboCop’s) for simplicity’s sake. We will have to think this over very carefully.

@baweaver

This comment has been minimized.

Copy link

baweaver commented Jan 31, 2019

If you need any help on this let me know. I was looking into doing something similar so I'd be more than willing to pitch in and help document the entire thing as well.

@jonatas

This comment has been minimized.

Copy link
Contributor Author

jonatas commented Feb 6, 2019

Got it @bbatsov. Looking at the code, we can see node_pattern depends on astx. We can do it in two gems or only one.

@baweaver let me know how can we schedule a pairing session or work together on it. I'm open to both cases. We can go with one or two gems. I'd start with one and we can split the external gem later if needed.

@pirj

This comment has been minimized.

Copy link
Member

pirj commented Mar 10, 2019

With the movement of extracting parts of RuboCop out of it (e.g. rubocop-performance), while keeping those extracted repositories first-class citizen of RuboCopHQ, it makes sense to think about how a change to main rubocop, and specifically to its node pattern repository would affect the dependent repositories.

An approach to consider is the one taken by RSpec, when all the repositories are checked out, and before submitting a change one would check if the dependent officially supported gems are not broken.

# rspec-expectations' Gemfile
branch = File.read(File.expand_path("../maintenance-branch", __FILE__)).chomp # `master`, or some cross-repository branch
%w[rspec rspec-core rspec-mocks rspec-support].each do |lib|
  library_path = File.expand_path("../../#{lib}", __FILE__)
  if File.exist?(library_path) && !ENV['USE_GIT_REPOS']
    gem lib, :path => library_path
  else
    gem lib, :git => "https://github.com/rspec/#{lib}.git", :branch => branch
  end
end

With that modular yet atomically safe approach, it doesn't matter that much anymore if rubocop-node_pattern is in the same repository or not, and breakages can be spotted early, during local development. That also kind of simplifies the changes that need to be made in several repositories at once to fix a bug/introduce a feature.

WDYT?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 10, 2019

Yeah, that's a reasonable approach. We should first finish the isolation/extraction of something though. :-) Right now everything's work in progress...

@jonas054 Any updates here?

@jonas054

This comment has been minimized.

Copy link
Collaborator

jonas054 commented Mar 10, 2019

@bbatsov It's very nice to hear from you again! 😄 But did you in this case mean @jonatas?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 10, 2019

Ops! Sorry about that! I totally meant to ping @jonatas, but I love you so much that I type your name every time I start typing @jona.... 😄

@jonatas

This comment has been minimized.

Copy link
Contributor Author

jonatas commented Mar 11, 2019

I like the idea and I guess we can try to follow the approach @pirj suggested.

The hardest part is that we have ast/node that is a dependency for node_pattern. I'll try to pair with @pirj tomorrow and see if we can handle it.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 11, 2019

Sounds good. Ping me if you need my help with anyone.

@jonatas

This comment has been minimized.

Copy link
Contributor Author

jonatas commented Mar 11, 2019

Thanks, @bbatsov. Should we keep the same module name for now or already go with astx?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 13, 2019

I guess for now it can be the same module name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.