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.

Sign up for GitHub

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

Jump to bottom

feature: support multiple prefixes #142

Merged
merged 1 commit into from Mar 2, 2023
Merged

feature: support multiple prefixes #142

merged 1 commit into from Mar 2, 2023

Conversation

ccoVeille
Copy link
Contributor

@ccoVeille ccoVeille commented Feb 25, 2023 •
edited

Fix #107

This PR includes the changes made in #134 by @luw2007

gci write -s standard -s default -s 'Prefix(alt.code.company.com,code.company.com)' --custom-order --skip-generated .

Should yield:

  "fmt"

  "github.com/daixiang0/gci"

  "alt.code.company.com/foo/boo"
  "code.company.com/foo/bar/baz"
)

@ccoVeille ccoVeille force-pushed the multiple-prefixes branch 2 times, most recently from ec8136d to 43e6c14 Compare February 25, 2023 01:10
@ccoVeille
Copy link
Contributor Author

I'm unsure about the way I documented it in the --help and in the README.md

I'm open to suggestion

I also fixed some typos.

I had no idea what could have been "presed", so I used "present"

@ccoVeille ccoVeille marked this pull request as ready for review February 25, 2023 01:21
@ccoVeille ccoVeille mentioned this pull request Feb 25, 2023
Closed
daixiang0
daixiang0 reviewed Feb 27, 2023
View reviewed changes
pkg/section/prefix.go Outdated Show resolved Hide resolved
@daixiang0
Copy link
Owner

I'm unsure about the way I documented it in the --help and in the README.md

I'm open to suggestion

I also fixed some typos.

I had no idea what could have been "presed", so I used "present"

"presed" means default used, we can remove it safely since we have mentioned the default enabled sections.

@ccoVeille
Copy link
Contributor Author

I'm unsure about the way I documented it in the --help and in the README.md
I'm open to suggestion
I also fixed some typos.
I had no idea what could have been "presed", so I used "present"

"presed" means default used, we can remove it safely since we have mentioned the default enabled sections.

I removed them, please read and tell me

daixiang0
daixiang0 reviewed Feb 28, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
daixiang0
daixiang0 reviewed Feb 28, 2023
View reviewed changes
pkg/section/prefix.go Outdated Show resolved Hide resolved
@daixiang0 daixiang0 changed the title Multiple prefixes feature: support multiple prefixes Feb 28, 2023
@ccoVeille
Copy link
Contributor Author

I updated the commit message, please review it

daixiang0
daixiang0 reviewed Mar 1, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
daixiang0
daixiang0 reviewed Mar 1, 2023
View reviewed changes
cmd/gci/gcicommand.go Outdated Show resolved Hide resolved
daixiang0
daixiang0 reviewed Mar 1, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@ccoVeille
Support multiple prefixes to group them together daixiang0#107
72599ab 
This changes allows to use this to group multiple prefixes

gci write -s standard -s default -s 'Prefix(alt.code.company.com,code.company.com)' --custom-order --skip-generated

In order to provide this, a change was needed in gci --section parameter parsing
The code was using pflag.StringSliceP for parsing sections.

Please consider the pflag documentation:

> Compared to StringArray flags, StringSlice flags take comma-separated value as arguments and split them accordingly.
> For example: --ss="v1,v2" --ss="v3"

While the usage of StringSliceP was legitimate with -local legacy parameter, if we consider gci documentation:

> -l, --local strings   put imports beginning with this string after 3rd-party packages, separate imports by comma

We can assume that when gci was rewritten in its "new style", it appears it was unintended to keep using pflag.StringSliceP.

Switching back to StringArrayP allows us to use comma in CLI arguments,
without this change the following command would have failed:

gci write -s standard -s default -s 'Prefix(alt.code.company.com,code.company.com)' --custom-order --skip-generated

because pflag.StringSliceP would have considered it as:

gci write -s standard -s default -s 'Prefix(alt.code.company.com' -s 'code.company.com)' --custom-order --skip-generated

We can assume this change will be OK. But it will break the following
command:

gci write -s standard,default

It was working because of StringSliceP

The syntax is now gci write -s standard -s default as documented

Signed-off-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
@ccoVeille
Copy link
Contributor Author

I changed according to your feedbacks

daixiang0
daixiang0 approved these changes Mar 2, 2023
View reviewed changes
Copy link
Owner

@daixiang0 daixiang0 left a comment

Choose a reason for hiding this comment

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

LGTM

@daixiang0 daixiang0 merged commit 65b147e into daixiang0:master Mar 2, 2023
@ImSingee ImSingee mentioned this pull request Mar 29, 2023
Closed
@deathbots deathbots mentioned this pull request Apr 13, 2023
Closed
4 tasks
@daixiang0 daixiang0 mentioned this pull request Apr 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daixiang0 daixiang0 daixiang0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiple prefixes to group them together and/or a regex
2 participants
@ccoVeille @daixiang0