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

Multi file, multi folder support #121

Closed
wants to merge 17 commits into from
Closed

Conversation

helmecke
Copy link
Contributor

Issue #47 , if available:

Description of changes:
Added support for multiple files in multiple folders.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@helmecke helmecke marked this pull request as ready for review December 16, 2019 11:04
@helmecke helmecke mentioned this pull request Dec 16, 2019
@knqyf263
Copy link
Owner

Thank you for the awesome contribution! Let me know why you removed config.go. Also, it would be appreciated if you show an example to use this new feature.

@helmecke
Copy link
Contributor Author

helmecke commented Jan 7, 2020

Thanks and sorry for the late reply.

Let me know why you removed config.go.

Viper is very intuetive to work with and integrates nicely with Cobra. It removes the need to maintain an additional file which changes every time the config is changed.

Also, it would be appreciated if you show an example to use this new feature.

I will add a section to the readme as soon as possible.

@helmecke
Copy link
Contributor Author

@knqyf263 I've added the Readme section. I would very much like your feedback.

@brandonkal
Copy link

There's a typo in the readme change.

@knqyf263
Copy link
Owner

knqyf263 commented Sep 8, 2020

I'm so sorry to be too late. I'm reviewing this PR.

Copy link
Owner

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Could you revert config.go? If we don't have it, we have to access each value with a string like viper.GetString("gitlab.file_name") everywhere as you did. This is likely to cause typos and is not desirable for completion. If you strongly hope it, let's open the dedicated issue and discuss it.

Comment on lines -1 to -14
.PHONY: \
all \
dep \
depup \
update \
build \
install \
lint \
vet \
fmt \
fmtcheck \
clean \
pretest \
test
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any reason to remove them?

)

// clipCmd represents the clip command
var clipCmd = &cobra.Command{
Copy link
Owner

Choose a reason for hiding this comment

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

This feature is not related to multi-folder support, right? If so, could you open another PR for this feature?

)

// completionCmd represents the completion command
var completionCmd = &cobra.Command{
Copy link
Owner

Choose a reason for hiding this comment

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

ditto

@helmecke
Copy link
Contributor Author

helmecke commented Sep 8, 2020

@knqyf263 it was my bad to open this PR from master branch. I'll try to cleanup this PR to only include multi-folder and multi-file support.

@helmecke helmecke closed this Sep 13, 2020
@helmecke helmecke deleted the master branch September 13, 2020 06:07
@helmecke helmecke restored the master branch September 13, 2020 06:16
@Piotr1215
Copy link

Hi everyone. Thank you for this great project! Is there any timeline on when this PR will be merged? I'm using @helmecke fork for now.

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