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

Flag ignores default value #99

Conversation

alecbakholdin
Copy link
Contributor

Resolves #98
Adds one new field to the arg interface "noDefault" that Flag uses

@alecbakholdin
Copy link
Contributor Author

alecbakholdin commented Jul 1, 2022

Not really sure why this is failing. Seems Travis CI can't find goveralls but it was definitely installed earlier in the build

$ $GOPATH/bin/goveralls -service=travis-ci
/home/travis/.travis/functions: line 109: /home/travis/gopath/bin/goveralls: No such file or directory
The command "$GOPATH/bin/goveralls -service=travis-ci" exited with 127.

Earlier in the build:

$ go get github.com/mattn/goveralls
go: downloading github.com/mattn/goveralls v0.0.11
go: downloading golang.org/x/mod v0.4.2
go: downloading golang.org/x/tools v0.1.1
go: downloading golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
go: added github.com/mattn/goveralls v0.0.11

@akamensky
Copy link
Owner

akamensky commented Jul 1, 2022

@Alec-Bakholdin thank you for contribution!

I think these build failures have to do with Go command line usage changes, let me have a look.

Edit:

Right, trying this on my desktop with same 1.18.3 installed:

(base) [akamenskiy@hostname ~]$ go install github.com/mattn/goveralls
go: 'go install' requires a version when current directory is not in a module
	Try 'go install github.com/mattn/goveralls@latest' to install the latest version
(base) [akamenskiy@hostname ~]$ go version
go version go1.18.3 linux/amd64

@akamensky
Copy link
Owner

Looks like the service is having issues. I created a PR to fix the original problem, but it appears the service is currently down and responding with 405 errors. I will check in on it later once they fixed issues on their side (and meantime I think it is worth changing to Github actions perhaps and see if they have coverage reporting within Github service). https://status.coveralls.io/

argument.go Outdated
@@ -501,6 +502,9 @@ func (o *arg) setDefaultFiles() error {
func (o *arg) setDefault() error {
// Only set default if it was not parsed, and default value was defined
if !o.parsed && o.opts != nil && o.opts.Default != nil {
if o.noDefault {
return fmt.Errorf("argument [%s, %s] does not support default values", o.sname, o.lname)
}
Copy link
Owner

Choose a reason for hiding this comment

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

While this solution would fix the problem, I think it may cause issues with any existing code, which should be avoided (i am trying to maintain this library backward compatible as much as possible). If some code was setting it to false which would make it work as expected, with this change that code will start failing.

I think better options that maintain some sort of compatibility is to either ignore Options.Default for Flag or to add logic for inverting Flag value (Options.Default defaults to false, if set to true then if flag present the value set to false, otherwise true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. What do you think about adding an argType field to the arg interface that makes it easier to introduce type-specific functionality like this? Currently, determining argument type is difficult down the line in functions like setDefault(). I think an enum would work really well, and would not change any existing behavior.

Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about adding an argType field to the arg interface that makes it easier to introduce type-specific functionality like this?

Yes, I were considering something like that for V2, but current schedule does not allow me to start working on it just yet. My concern with the current one is just the backward compatibility, this lib is used by a number of tools, and would not want to introduce a breaking change here. So any solution you like that would not break existing code outright should be okay I think.

@akamensky
Copy link
Owner

@Alec-Bakholdin Please rebase the change onto master to fix coverage reporting

@akamensky
Copy link
Owner

@Alec-Bakholdin I am sorry to be pedantic about this, can you please avoid cross merging master branch and use rebase instead?

@alecbakholdin
Copy link
Contributor Author

alecbakholdin commented Jul 6, 2022

Not at all! I'm sorry, I'm not familiar with this process, as this is my first contribution ever. I tried to rebase once more, but I think I may have screwed things up again...

I thought I was doing it right. Is the process not to just type git rebase upstream/master from my own branch?

@akamensky akamensky changed the base branch from master to flag-ignores-default-value July 28, 2022 12:33
@akamensky akamensky merged commit f6635fb into akamensky:flag-ignores-default-value Jul 28, 2022
@akamensky
Copy link
Owner

Merged to master in #103

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.

Options.Default should be ignored for Flag
2 participants