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

Handle multiple annotations repositories #955

Merged
merged 7 commits into from
Jun 1, 2022
Merged

Handle multiple annotations repositories #955

merged 7 commits into from
Jun 1, 2022

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented May 31, 2022

Motivation

Allow user to use multiple sources for annotations:

tapioca annotations --repo-uri URI1 URI2

Implementation

  1. First we pull each repo's index.
  2. For each gem in the Gemfile.lock we check which indexes contain annotations files for the gem
  3. Pull all the annotations files from all the repos containing one for the gem
  4. Merge the annotations files into a single one

In case one of the annotation files contains a parse error, we tell so to the user and skip the gem.
In case one of annotations files can't be merged because of conflicts, we tell so to the user and skip the gem.

Tests

See automated tests.

@Morriar Morriar added enhancement New feature or request feature labels May 31, 2022
@Morriar Morriar requested a review from a team May 31, 2022 19:51
@Morriar Morriar self-assigned this May 31, 2022
@@ -302,9 +302,9 @@ def check_shims
end

desc "annotations", "Pull gem annotations from a central RBI repository"
option :repo_uri, type: :string, desc: "Repository URI to pull annotations from", default: CENTRAL_REPO_ROOT_URI
option :repo_uri, type: :array, desc: "Repository URI to pull annotations from", default: [CENTRAL_REPO_ROOT_URI]
Copy link
Member

Choose a reason for hiding this comment

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

Since it accepts an array, could we change the option name to plural (repo_uris) and also update the description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed it as --repos (98663d1) let me know what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I changed it to --sources (8cf24a0)

lib/tapioca/commands/annotations.rb Outdated Show resolved Hide resolved
lib/tapioca/commands/annotations.rb Show resolved Hide resolved
@Morriar Morriar changed the title Handle multiple Handle multiple annotations repositories May 31, 2022
@Morriar Morriar force-pushed the at-multi-repos branch 2 times, most recently from 98663d1 to 47539bd Compare May 31, 2022 22:04
@Morriar
Copy link
Collaborator Author

Morriar commented May 31, 2022

I also added some documentation for the command: 47539bd.

fetchable_gems = T.let(gem_names.each_with_object({}) do |gem_name, hash|
@indexes.each do |uri, index|
if index.has_gem?(gem_name)
hash[gem_name] ||= []
Copy link
Member

Choose a reason for hiding this comment

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

NIT: you can use a hash with a default of an array to avoid this line and possibly simplify the loop.

Hash.new { |h, k| h[k] = [] }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I also simplified the loop 👍

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

Do you think allowing multiple repos will hinder contributions to rbi-central?

From that perspective I wasn't sure if it's a necessary feature without support for private repos.

Morriar added 7 commits June 1, 2022 11:47
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar
Copy link
Collaborator Author

Morriar commented Jun 1, 2022

@KaanOzkan:

Do you think allowing multiple repos will hinder contributions to rbi-central?

I think it may actually be the opposite.

With only one repo, as soon as you have private things you'll want a private repo and be forced to use just this one meaning no contribution to the community one.

Even without private things, if for some reason you want to use your own repo for something custom that shouldn't be in the community one (let's say some annotations for a kind of static DSL you use) you'll also depend on your own repo and not contribute to the community one.

By allowing people to use multiple repo we give them the opportunity to use and contribute to the community repo while at the same time maintaining their own annotations.

So enabling multiple (public) repos seemed to be the next logical step before adding support to private repos.

@Morriar Morriar merged commit ede7d33 into main Jun 1, 2022
@Morriar Morriar deleted the at-multi-repos branch June 1, 2022 20:51
@shopify-shipit shopify-shipit bot temporarily deployed to production July 7, 2022 17:53 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants