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

Refactor collection #1149

Merged
merged 16 commits into from
Dec 5, 2022
Merged

Refactor collection #1149

merged 16 commits into from
Dec 5, 2022

Conversation

soutaro
Copy link
Member

@soutaro soutaro commented Nov 16, 2022

This is a step toward #1148, it refactor the Ruby code and RBS type definition.

  • More strict Sources::* method types, to declare if version is required or not.
  • Collection::Config::Lockfile is introduced to make rbs_collection.lock.yaml data structure clear.
  • Sources::Git is refactored that reduces git checkout operation as much as possible

The Collection::* API changes a lot. Not very sure if we need to bump the major version of the gem...

@soutaro soutaro requested a review from pocke November 16, 2022 07:26
@pocke
Copy link
Member

pocke commented Nov 21, 2022

The Collection::* API changes a lot. Not very sure if we need to bump the major version of the gem...

I think we don't need to bump the major version. Because rbs collection CLI command is only intended to expose end users, I do not intend to expose other APIs such as RBS::Collection::* for end users.

edit: I forgot Steep and TypeProf. They refer RBS::Collection a little. But it is only a small dependency so I guess we do not need to bump the major version.

Comment on lines 45 to 50
{
"name" => source.name,
"remote" => source.remote,
"revision" => source.resolved_revision,
"repo_dir" => source.repo_dir
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this replaceable with Source#to_lockfile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? But, the Source#to_lockfile includes "type" while existing lockfiles don't.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think it's not a problem because it's not a breaking change. Including type is more descriptive, and we can keep it more DRY.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will have type attribute. We are planning to add another type of sources soon. So having git type here is nice.

# Conflicts:
#	lib/rbs/collection/config/lockfile_generator.rb
#	lib/rbs/collection/sources/base.rb
#	lib/rbs/collection/sources/stdlib.rb
#	lib/rbs/environment_loader.rb
#	sig/collection/sources.rbs
#	test/rbs/collection/sources/stdlib_test.rb
* Use `Lockfile` instead of re-using `Config` class
* `Config` is the user edited configuration file — `rbs_collection.yaml`
* `Lockfile` is the generated lockfile — `rbs_collection.lock.yaml`
* Reduces `git checkout` operation as much as possible
* Stop issuing `git fetch` from `#initialize`
@soutaro soutaro force-pushed the refactor-collection branch from 6e29d76 to 7cd6aac Compare December 5, 2022 07:17
@soutaro soutaro enabled auto-merge December 5, 2022 07:17
@soutaro soutaro merged commit e926a9a into master Dec 5, 2022
@soutaro soutaro deleted the refactor-collection branch December 5, 2022 07:23
@soutaro soutaro added this to the RBS 3.0 milestone Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants