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

Feature: (issue_1451) customized slice flag separator #1546

Merged

Conversation

FGYFFFF
Copy link
Contributor

@FGYFFFF FGYFFFF commented Oct 24, 2022

What type of PR is this?

feature
(REQUIRED)

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

add customized slice flag separator
(REQUIRED)

Which issue(s) this PR fixes:

Feature #1541
(REQUIRED)

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Release Notes

(REQUIRED)

Feature: (https://github.com/urfave/cli/issues/1541)add  customized slice flag separator

@FGYFFFF FGYFFFF requested a review from a team as a code owner October 24, 2022 06:12
@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch from 53f1b5e to 2d390e9 Compare October 24, 2022 06:13
@FGYFFFF FGYFFFF changed the title feat: customized slice flag separator Feature: customized slice flag separator Oct 24, 2022
@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Oct 24, 2022

The setting of "SliceFlagSeparator" is "global-level" because it is necessary to keep the same separator for each "sliceFlag". Therefore it is not set to "flag-level"

@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch 8 times, most recently from b012801 to ba36536 Compare October 24, 2022 07:53
@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Oct 24, 2022

@dearchap Please help me, why does "CI" keep failing?😭

@dearchap
Copy link
Contributor

Can you do a gofmt or goimports in main dir ?

@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch from ba36536 to c3675c0 Compare October 24, 2022 09:04
@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Oct 24, 2022

Can you do a gofmt or goimports in main dir ?

I did gofmt&goimports for the "flag_test.go" file, but it still failed.

@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch 7 times, most recently from dffd74a to f71906f Compare October 25, 2022 03:45
@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch 5 times, most recently from 282d0a0 to 2f10ac7 Compare October 25, 2022 06:44
@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Oct 25, 2022

@dearchap Please help me review the code, I rely on this feature to do my work!💕💕💕

@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch from 2f10ac7 to c5da5f6 Compare October 25, 2022 06:59
@FGYFFFF FGYFFFF changed the title Feature: customized slice flag separator Feature: (issue_1451) customized slice flag separator Oct 25, 2022
@FGYFFFF FGYFFFF requested a review from dearchap October 25, 2022 11:39
flag_test.go Outdated
}

_ = app.Run(os.Args)
defaultSliceFlagSeparator = ","
Copy link
Contributor

Choose a reason for hiding this comment

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

better to add the revert of defaultSliceFlagSeparator in a defer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

flag_test.go Outdated
}
app.SliceFlagSeparator = ";"
app.Action = func(ctx *Context) error {
ret := ctx.StringSlice("stringSlice")
Copy link
Contributor

Choose a reason for hiding this comment

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

its better not to rely on Action here. If action doesnt get called the test would pass. why dont you set the destination filed of the string slice flag and then validate the slice contents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! ✌️

@FGYFFFF FGYFFFF force-pushed the feat/disable_split_parameters_for_slice_flag branch from ad7dfde to 957957f Compare October 27, 2022 07:39
@FGYFFFF FGYFFFF merged commit 63cb372 into urfave:main Oct 27, 2022
12 of 13 checks passed
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.

None yet

2 participants