Skip to content

Conversation

onetonfoot
Copy link
Contributor

No description provided.

@mcmcgrath13
Copy link
Collaborator

Thanks! Could you also update the type generation uses of JSON.read to no longer do the file checks? Seems like it shouldn't be necessary anymore with this change.

@mcmcgrath13
Copy link
Collaborator

Sorry for the add on, but could you also add a test for this?

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Base: 88.36% // Head: 88.37% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (efb0b0c) compared to base (8569ff9).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head efb0b0c differs from pull request most recent head 2ae5e08. Consider uploading reports for the commit 2ae5e08 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #243   +/-   ##
=======================================
  Coverage   88.36%   88.37%           
=======================================
  Files           9        9           
  Lines        1728     1729    +1     
=======================================
+ Hits         1527     1528    +1     
  Misses        201      201           
Impacted Files Coverage Δ
src/read.jl 88.62% <100.00%> (+0.03%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@onetonfoot
Copy link
Contributor Author

Good thing you asked for some test cases, it wasn't behaving as expected when reading types

@rafaqz
Copy link

rafaqz commented Dec 19, 2022

Any chance we can get this merged?

json = read_json_str(json_str)

# build a type for the JSON
json = read(json_str)
Copy link
Owner

Choose a reason for hiding this comment

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

wait, why are we not using read_json_str here anymore?

Copy link
Collaborator

Choose a reason for hiding this comment

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

read now does what read_json_str did, so it's redundant

Copy link
Owner

Choose a reason for hiding this comment

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

ah got it.

@quinnj
Copy link
Owner

quinnj commented Jan 4, 2023

@mcmcgrath13 does this look good to you? I think it LGTM

Copy link
Collaborator

@mcmcgrath13 mcmcgrath13 left a comment

Choose a reason for hiding this comment

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

LGTM - Thank you!

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.

4 participants