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

Add project paths command to LF #495

Merged
merged 7 commits into from
Jun 5, 2018
Merged

Conversation

xtreme-shane-lattanzio
Copy link
Contributor

Signed-off-by: Daniil Kouznetsov dkouznetsov@pivotal.io

xtreme-shane-lattanzio and others added 2 commits May 31, 2018 15:58
Signed-off-by: Daniil Kouznetsov <dkouznetsov@pivotal.io>
Signed-off-by: Daniil Kouznetsov <dkouznetsov@pivotal.io>
@@ -41,7 +42,7 @@ def project_root?(pathname)

def active_project?(project_path)
active_project = @package_managers.map do |pm|
pm.new(project_path: project_path).active?
pm.new(project_path: project_path).active? unless pm == GoWorkspace && @configurable
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the break in polymorphism? That is, why not pass @configurable to pm.new, and let GoWorkspace do something with it?

A related problem is that the name @configurable doesn't imply what it will be used for. Instead, maybe you could add an argument (possibly exposed at the CLI) which says which types of project managers to skip.

Copy link

@xtreme-vikram-yadav xtreme-vikram-yadav left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 👍

@@ -98,4 +98,30 @@
expect(developer).to_not be_seeing('MIT')
end
end

context 'running project_roots', :focus do

Choose a reason for hiding this comment

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

:focus .

@@ -98,4 +98,30 @@
expect(developer).to_not be_seeing('MIT')
end
end

context 'running project_roots', :focus do

Choose a reason for hiding this comment

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

can you update this to a describe?

@@ -158,11 +164,12 @@ def check_valid_project_path
raise "Project path '#{config.project_path}' does not exist!" unless config.valid_project_path?
end

def aggregate_paths
def aggregate_paths(strict_matching = false)

Choose a reason for hiding this comment

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

Can we use the config to do this instead? That way we avoid having a parameter on this method just to pass it to the project finder.

Signed-off-by: Yoon Jean Kim <ykim@pivotal.io>
Signed-off-by: Yoon Jean Kim <ykim@pivotal.io>
@xtreme-shane-lattanzio xtreme-shane-lattanzio merged commit b7a22ea into master Jun 5, 2018
@xtreme-vikram-yadav xtreme-vikram-yadav deleted the project_paths branch January 11, 2019 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants