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

Context PostValueIntDefault method should be return (int, error) ? #937

Closed
jefurry opened this issue Mar 21, 2018 · 8 comments
Closed

Context PostValueIntDefault method should be return (int, error) ? #937

jefurry opened this issue Mar 21, 2018 · 8 comments
Labels

Comments

@jefurry
Copy link

jefurry commented Mar 21, 2018

iris/context/context.go

i think PostValueIntDefault method should be return int. if happend error, given it zero.

same as PostValueInt64Default, PostValueFloat64Default, URLParamIntDefault, URLParamInt64Default, URLParamFloat64Default etc. because i want to get a value only.

@kataras
Copy link
Owner

kataras commented Mar 21, 2018

Hello @jefurry,

The implementation you're asking for is absolutely logical and I was thinking the same when I decide to add them.

The PostValueXXXDefault methods should do that you're describing of. To be honest, they were added for that reason but before push I thought it a bit further and I selected to return the error as well, because you may want to know if it's a parse error (0 value and the error returned) or just not found by the request (the default value the caller passed and a nil error).

Unfortunately (?) I can't change it now, because it will bring breaking changes.
Both PostValueXXX(key) (XXX, error) and PostValueXXXDefault(key, defaultValue) (XXX, error) are very useful, including the returned error of the second.

What we can do instead? We can create new functions to complete your request, if you're OK with this, you can also recommend some names for those :)

Thanks,
Gerasimos Maropoulos

@kataras kataras added good first issue A user wrote a good first issue with clear instructions 💡type:idea labels Mar 21, 2018
@jefurry
Copy link
Author

jefurry commented Mar 22, 2018

I think there was a problem at the beginning.

PostValueXXX is not need to a default value, if want to catch any error that using PostValueXXX otherwise using PostValueXXXDefault. now It's confusing!

func (ctx *context) PostValueIntDefault(name string, def int) (int, error) {
	v := ctx.PostValue(name)
	if v == "" {
		return def, nil
	}
	return strconv.Atoi(v)
}

func (ctx *context) PostValueInt(name string) (int, error) {
	return ctx.PostValueIntDefault(name, 0)
}

change to:

func (ctx *context) PostValueInt(name string) (int, error) {
	return strconv.Atoi(ctx.PostValueTrim(name))
}

func (ctx *context) PostValueIntDefault(name string, def int) int {
	if v, err := ctx.PostValueInt(name); err == nil {
            return v
        }

        return def
}

As you said, it will bring breaking changes if you change it now. But you will eventually do it!

@jefurry
Copy link
Author

jefurry commented Mar 22, 2018

ViewData exposed too much details. nobody using it like this:

ctx.ViewData("title", "this is title")
...
ctx.ViewData(...)

and View method should be:

func (ctx *context) View(filename string, optionalBindingData ...interface{}) error {
    ...
    if len(optionalBindingData) > 0 {
        ctx.ViewData("", optionalBindingData)
    }
    bindingData := ctx.values.Get(cfg.GetViewDataContextKey())
    ...
}

we hope so.

raw body should be cached. because sometimes we want to deal with it ourselves!

@kataras
Copy link
Owner

kataras commented Mar 23, 2018

OK @jefurry so we have to propose a breaking API change there, I am fine with it, practically you have right, I had too but we will have some issues with users post this as "bug" so we have to help them as well (although we could just keep the XXXDefault which will return a default value and a non-nil error if not found or parse errors as well). The result will be the same at the end but more expected the source code, nice. The ViewData is used by many people, you're wrong about it, its code is fine, we don't have any issues there, it was a user proposal in order to modify view data from other handlers in the request chain.

@kataras
Copy link
Owner

kataras commented Mar 23, 2018

This will change the sessions.GetIntDefault as well.

@kataras
Copy link
Owner

kataras commented Mar 24, 2018

And the memstore.go which is the ctx.Values() API.

@kataras
Copy link
Owner

kataras commented Mar 24, 2018

Done @jefurry , upgrade to the latest version, 10.5.0, with go get -u github.com/kataras/iris, the commit: 41b17ac. Example change:

old:
count,_ := c.Session.GetIntDefault("count", 1)

now (>=v10.5):
count := c.Session.GetIntDefault("count", 1)

@kataras kataras closed this as completed Mar 24, 2018
@jefurry
Copy link
Author

jefurry commented Mar 24, 2018

@kataras

i mean that ViewData exposed too much details, such as ViewData("key", "value"), ViewData("", ...) etc.

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants