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

More concrete Gadget2 format detector #351

Merged
merged 3 commits into from Oct 30, 2021
Merged

Conversation

islent
Copy link
Contributor

@islent islent commented Oct 26, 2021

Probably fix #350

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #351 (d476385) into master (e04d83d) will increase coverage by 0.89%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   87.44%   88.34%   +0.89%     
==========================================
  Files          10       10              
  Lines         693      695       +2     
==========================================
+ Hits          606      614       +8     
+ Misses         87       81       -6     
Impacted Files Coverage Δ
src/registry.jl 93.22% <100.00%> (+3.38%) ⬆️
src/query.jl 96.99% <0.00%> (+0.02%) ⬆️
src/registry_setup.jl 95.79% <0.00%> (+0.03%) ⬆️

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 e04d83d...d476385. Read the comment docs.

@johnnychen94
Copy link
Member

@junglegobs can you help verify this?

@timholy
Copy link
Member

timholy commented Oct 28, 2021

Thanks @islent !

@junglegobs can you help verify this?

We wouldn't need help if someone would contribute an actual test file for Gadget2 😄.

Let's wait a few more days for confirmation (I'm unfamiliar with this format) and if we don't hear back, let's merge.

@islent
Copy link
Contributor Author

islent commented Oct 28, 2021

Hello, I've uploaded the test file, and implemented a simple testset.
785c5e6#diff-4d1be1e202c2e00168e38c662330eb9f41661071b5bc947c6b1f0e83150a2cc3

test/query.jl Outdated Show resolved Hide resolved
@islent
Copy link
Contributor Author

islent commented Oct 29, 2021

Need help :D. How could tests on nightly build fail? Should we care about that?

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

I don't know Gadget2 format so I'm not sure if this breaks people's workflow. But I also don't think my unfamiliarity should block this PR.

The test failure on nightly builds might be unrelated upstream issues. I also noticed that Pkg fails https://github.com/JuliaLang/Pkg.jl/runs/4029907658?check_suite_focus=true

@johnnychen94
Copy link
Member

Let's test this in the wild.

@johnnychen94 johnnychen94 merged commit 740638d into JuliaIO:master Oct 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.

AstroIO required when it shouldn't be
3 participants