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

fix: you could not use a struct to for both di and json marshalling. #41

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

chirino
Copy link
Contributor

@chirino chirino commented Nov 26, 2021

You must at least add an empty di:"" tag to struct fields to also support json marshalling. Added test case to verify it works.

You must at least add an empty di:"" tag to struct fields to also support json marshalling. Added test case to verify it works.
@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #41 (4a262a6) into master (3325231) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   95.06%   95.08%   +0.01%     
==========================================
  Files          16       16              
  Lines         730      732       +2     
==========================================
+ Hits          694      696       +2     
  Misses         24       24              
  Partials       12       12              
Impacted Files Coverage Δ
inject.go 88.33% <100.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3325231...4a262a6. Read the comment docs.

@defval
Copy link
Owner

defval commented Nov 29, 2021

Hi, @chirino! This change breaks backward compatibility. And some time ago we already refused empty "di" tags. Because we didn't think that program components could be serialized.

Maybe you should separate the data structures containing dependencies and configuration?

@chirino
Copy link
Contributor Author

chirino commented Nov 29, 2021

Do you have any examples of how this might break existing apps? Seems to me if it empty tags were previously not allowed, then there should be no previous apps that are using empty di tag.

@defval
Copy link
Owner

defval commented Nov 29, 2021

They've been there before.

Sorry. I remembered that you were doing support for the old syntax. And our internal cases are exactly covered with tests.

We can merge it.

@defval defval merged commit 2e5190d into defval:master Nov 29, 2021
@defval
Copy link
Owner

defval commented Nov 30, 2021

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.

2 participants