quinnj / JSON3.jl

Other
214 stars 47 forks source link

Read files directly #243

Closed onetonfoot closed 1 year ago

mcmcgrath13 commented 1 year ago

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 commented 1 year ago

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

codecov[bot] commented 1 year ago

Codecov Report

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

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

:exclamation: 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 ```diff @@ 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](https://codecov.io/gh/quinnj/JSON3.jl/pull/243?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jacob+Quinn) | Coverage Δ | | |---|---|---| | [src/read.jl](https://codecov.io/gh/quinnj/JSON3.jl/pull/243/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jacob+Quinn#diff-c3JjL3JlYWQuamw=) | `88.62% <100.00%> (+0.03%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jacob+Quinn). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jacob+Quinn)

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

onetonfoot commented 1 year ago

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

rafaqz commented 1 year ago

Any chance we can get this merged?

quinnj commented 1 year ago

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