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 default tag #183

Merged
merged 5 commits into from
Feb 10, 2024
Merged

Add default tag #183

merged 5 commits into from
Feb 10, 2024

Conversation

zak905
Copy link
Contributor

@zak905 zak905 commented Aug 8, 2021

Summary of Changes

this is the implementation for the functionality described in #182

  1. add support for an optional default tag to allow setting values when decoding in case no value is provided
  2. limiting the default tag scope to only primitive types and their pointers as well

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

@stale stale bot added the stale label Jan 9, 2022
@zak905
Copy link
Contributor Author

zak905 commented Jan 9, 2022

Hello Gorilla Maintainers,

@kisielk
@elithrar
@moraes
@zeebo

any chance getting this reviewed or at least getting a feedback ? I am already using this directly from my own fork, works great so far

@jaitaiwan
Copy link
Member

I took a quick glance over this, nothing stands out to me. @zak905 I know it's been awhile, are you willing to update the PR with the latest base branch?

@zak905
Copy link
Contributor Author

zak905 commented Jan 18, 2024

@jaitaiwan done. thanks for looking into it. I am glad to see the project active again

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (4890efc) 83.23% compared to head (7807359) 86.59%.

Files Patch % Lines
decoder.go 92.45% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
+ Coverage   83.23%   86.59%   +3.35%     
==========================================
  Files           4        4              
  Lines         710      843     +133     
==========================================
+ Hits          591      730     +139     
+ Misses        103       96       -7     
- Partials       16       17       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AlexVulaj
Copy link
Member

Thanks so much for the contribution @zak905 - I hate to ask any more of you when this has already been in progress for so long, but is there any chance the tests could be updated to meet our coverage standards? I'll make a special note to watch out for this PR to get it in asap.

@zak905
Copy link
Contributor Author

zak905 commented Jan 24, 2024

Sure thing, I will look into it asap. Thanks.

@zak905
Copy link
Contributor Author

zak905 commented Feb 2, 2024

@AlexVulaj thanks again for looking into this. I have added more testing to cover the unreached code, it should be ok now hopefully

@AlexVulaj
Copy link
Member

@jaitaiwan I'm good with this, wanna give it a quick last glance and merge?

Copy link
Member

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

LGTM

decoder.go Show resolved Hide resolved
@jaitaiwan jaitaiwan merged commit eda9c33 into gorilla:main Feb 10, 2024
12 checks passed
@jaitaiwan
Copy link
Member

All finished @zak905 , appreciate the contribution.

@zak905
Copy link
Contributor Author

zak905 commented Feb 10, 2024

Welcome. Thanks @AlexVulaj and @jaitaiwan. I have to admit that I thought that this will never get merged, especially after the Gorilla project got archived :) so I would say this a great twist. Hopefully this will make setting default values trivial - I am hoping to see also some similar functionality in the standard golang json package someday.

By the way, do you think it's worth adding a README section about the default tag option ?

@jaitaiwan
Copy link
Member

Definitely, if you’re happy to make a contribution we’ll endeavour to look asap.

@zak905
Copy link
Contributor Author

zak905 commented Feb 19, 2024

thanks. Read me updated at: #209

@zak905
Copy link
Contributor Author

zak905 commented Mar 26, 2024

@jaitaiwan is it possible to create a release for this changeset (and other changes that occured since v1.2.1) ?

@jaitaiwan
Copy link
Member

Definitely possible @zak905

@zak905
Copy link
Contributor Author

zak905 commented Mar 27, 2024

awesome thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants