Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

[WIP]Add pristine command and spec #4729

Closed
wants to merge 2 commits into from

Conversation

allenzhao
Copy link
Contributor

@allenzhao allenzhao commented Jun 27, 2016

  • [Spec] Add spec for pristine all gems
  • [Spec] Add spec for pristine only a list of gems
  • [Spec] Add spec for pristine skip a list of gems
  • [Spec] Add spec for pristine extensions only
  • [Spec] Add spec for pristine no-extensions only
  • [Spec] Add spec for handling git-source gems
  • [Logic] Add logic for handling git-source gems
  • [Logic] Add logic for handling path-source gems
  • [Logic] Add logic to compute gem list to run gem pristine
  • [Logic] Add logic to handle extension related gems

@@ -0,0 +1,57 @@
# frozen_string_literal: true
module Bundler
class CLI::Pristine
Copy link
Member

Choose a reason for hiding this comment

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

please nest the class inside the namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't I already done the nesting?

Copy link
Member

Choose a reason for hiding this comment

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

never mind, this seems to be how the other CLI classes do it

@allenzhao allenzhao force-pushed the pristine-spec branch 2 times, most recently from bb1d816 to 1449b8b Compare June 28, 2016 03:49
@@ -42,6 +42,7 @@ class LockfileError < BundlerError; status_code(20); end
class CyclicDependencyError < BundlerError; status_code(21); end
class GemfileLockNotFound < BundlerError; status_code(22); end
class PluginError < BundlerError; status_code(23); end
class PristineError < BundlerError; end
Copy link
Member

Choose a reason for hiding this comment

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

needs a status code

@homu
Copy link
Contributor

homu commented Jun 28, 2016

☔ The latest upstream changes (presumably #4732) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Jul 4, 2016

☔ The latest upstream changes (presumably #4632) made this pull request unmergeable. Please resolve the merge conflicts.

@allenzhao allenzhao force-pushed the pristine-spec branch 2 times, most recently from f0b087b to 3be4738 Compare July 4, 2016 23:02
def pristine(gems, git_gems = nil, lazy_spec_provided = false)
gem_list = lazy_spec_provided ? gems.map(&:name) : gems
check_conflicting(gem_list, options[:skip])
pristine_gems = gem_list - options[:skip]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RochesterinNYC
To make line 53 & this line work I need to implement a "-" and a "&" on SpecSet since gem_list here could be an Array or an instance of SpecSet and options[:skip] would always be array. But I don't know if this would be somehow "bad practice" ? What's your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Casting and then performing the operations could be a possibility? Why can gem_list be either an array or a SpecSet instead of always one or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's not provided by the user, it's from the definition's locked_spec so it is a SpecSet and using a SpecSet rather than an array could provide the version information

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that a - and & method on the actual SpecSet itself is the right solution. Maybe write a method that handles the casting and comparison and then at the end, we can gauge whether it really belongs on the class itself?

@allenzhao allenzhao force-pushed the pristine-spec branch 2 times, most recently from 392c0a5 to b523d9a Compare July 9, 2016 01:40
@homu
Copy link
Contributor

homu commented Jul 9, 2016

☔ The latest upstream changes (presumably #4676) made this pull request unmergeable. Please resolve the merge conflicts.

@allenzhao allenzhao force-pushed the pristine-spec branch 3 times, most recently from e691e0f to b01acd9 Compare July 10, 2016 02:21
@homu
Copy link
Contributor

homu commented Jul 20, 2016

☔ The latest upstream changes (presumably #4770) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Aug 4, 2016

☔ The latest upstream changes (presumably #4765) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Aug 16, 2016

☔ The latest upstream changes (presumably #4877) made this pull request unmergeable. Please resolve the merge conflicts.

Change from bundle exec to gem pristine
@homu
Copy link
Contributor

homu commented Aug 20, 2016

☔ The latest upstream changes (presumably #4891) made this pull request unmergeable. Please resolve the merge conflicts.

@segiddins
Copy link
Member

@allenzhao what's the status on this?

gem_list = lazy_spec_provided ? gems.map(&:name) : gems
skip_gems = options[:skip]
pristine_gems = compute_pristine_gems(gem_list, skip_gems)
binding.pry
Copy link

@ruba-ruba ruba-ruba Nov 6, 2016

Choose a reason for hiding this comment

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

is pry here intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's still a WIP.

@allenzhao
Copy link
Contributor Author

@segiddins I haven't been able to pick up these PRs. I will manage to start working on them this week, but first I think I need some time to follow up on the current status of bundler dev.

@segiddins
Copy link
Member

Closed by #5503.

@segiddins segiddins closed this Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants