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

Add support for pointer fields. #280

Closed
wants to merge 1 commit into from
Closed

Conversation

scottwis
Copy link

No description provided.

kong_test.go Show resolved Hide resolved
mapper.go Outdated Show resolved Hide resolved
@haines
Copy link
Contributor

haines commented Mar 18, 2022

I think the check here needs to be updated to account for the possibility of *bool fields (otherwise you can't make a *bool field negatable):

kong/tag.go

Line 174 in 556f8b7

isBool = typ.Kind() == reflect.Bool

Also here:

kong/model.go

Line 311 in deebf0b

return v.Target.Kind() == reflect.Bool

@scottwis
Copy link
Author

@haines I think bool pointers should work just fine. I added some tests for various bool* cases. Let me know if there's something I missed.

@haines
Copy link
Contributor

haines commented Mar 19, 2022

@scottwis could you please check if you can use the negatable:"" tag with *bool fields (might be worth adding a test case for it)?

@scottwis
Copy link
Author

@alecthomas @haines @shueybubbles

I believe I have all comments addressed. Can you please take another look?

Copy link
Contributor

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

I'll leverage this!
thx

Copy link
Contributor

@haines haines left a comment

Choose a reason for hiding this comment

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

Thanks @scottwis, being able to distinguish between --foo, --no-foo, and neither is super helpful!

@alecthomas
Copy link
Owner

Ping. This is not passing lint.

@jh125486
Copy link
Sponsor Contributor

jh125486 commented Apr 7, 2022

@scottwis should I fix this (either //nolint or remove the fields) and push it?

@stevekm
Copy link

stevekm commented Jun 16, 2022

I needed this exact feature, hope it can get merged in soon. Thanks for implementing this :)

@alecthomas alecthomas closed this Sep 20, 2022
@alecthomas
Copy link
Owner

This was merged in #296

@alecthomas
Copy link
Owner

Thanks for all the work and apologies for the delay in responding.

@stevekm
Copy link

stevekm commented Dec 20, 2022

this is perfect thanks so much

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

6 participants