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

Support conversions from Map to Object (#39) #41

Closed
wants to merge 1 commit into from
Closed

Support conversions from Map to Object (#39) #41

wants to merge 1 commit into from

Conversation

davecardwell
Copy link

@davecardwell davecardwell commented Feb 13, 2020

Converting from a Map to an Object seems to be substantially the same as converting from an Object. The only difference is that converting from a Map must be unsafe as we don’t know if it has all of the necessary keys until we try to do the conversion.

I attempted to share as much of conversionObjectToObject as possible, then duplicated all of the Object-to-Object tests as Map-to-Object.

This is my first time writing go so please let me know if I haven’t done things in the idiomatic way, or have missed some potential pitfall.

Fixes #39.

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #41 into master will increase coverage by 0.04%.
The diff coverage is 82.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   76.08%   76.13%   +0.04%     
==========================================
  Files          77       77              
  Lines        6172     6193      +21     
==========================================
+ Hits         4696     4715      +19     
  Misses       1060     1060              
- Partials      416      418       +2
Impacted Files Coverage Δ
cty/convert/conversion.go 82.29% <50%> (-1.41%) ⬇️
cty/convert/conversion_object.go 92% <89.47%> (-1.94%) ⬇️
cty/function/stdlib/format_fsm.go 92.56% <0%> (ø) ⬆️

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 c282fd1...bfc321d. Read the comment docs.

@apparentlymart
Copy link
Collaborator

Hi @davecardwell! Thanks for working on this.

First I want to apologize for the delay in getting back to you on this. I've been getting caught back up from a vacation so I'm quite behind on processing my notifications. 😖

An unfortunate consequence of that delay is that I didn't notice until now that both you and @jbardin both worked on solutions to this problem at the same time, with the other in #42. Because I was working through the new items in reverse order of recentness, I saw, reviewed, and merged the other one first.

It looks like the two changesets achieve the same result (with the other one differing slight now based on my review), so I'm going to close this one. I'm sorry I didn't notice this one sooner, and thus that I must now close this without merging it.

Thanks again for working on it!

@davecardwell
Copy link
Author

davecardwell commented Feb 19, 2020

@apparentlymart Hey no problem, just glad it got done! Thanks for merging :)

@davecardwell davecardwell deleted the feature-39-map_to_object branch February 19, 2020 03:03
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.

Converting from Map to Object
2 participants