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 UTF8 string encoding #101

Closed
wants to merge 2 commits into from
Closed

fix UTF8 string encoding #101

wants to merge 2 commits into from

Conversation

ilia-kats
Copy link
Contributor

h5createAttribute/h5writeAttribute needed the argument to be "UTF-8" and passed it to H5Tset_cset, which needed the argument to be "UTF8". Therefore, setting the encoding to UTF-8 failed. This patch changes h5createAttribute and h5writeAttribute to accept "UTF8".

@grimbough
Copy link
Owner

Thanks a lot for pointing this out. I'm inclined to go the other way and use the "UTF-8" notation, principle because that's the official name of the encoding.

I see there's also some inconsistencies where this is set either via the cset or encoding arguments depending on whether its and attribute or a dataset that's being created. I'll think I'll set both to use encoding going forward, but I'll only deprecated the cset argument so it'll still work if you're currently using it.

@ilia-kats
Copy link
Contributor Author

Thanks for the feedback, I updated the PR to use UTF-8instead of UTF8.

@grimbough
Copy link
Owner

grimbough commented Nov 29, 2021

Did you see that I made a pull request on your own repo with some additional changes? Those pass R CMD check (
https://github.com/grimbough/rhdf5/actions/runs/1515325296)

It'd be good if you could look over those, so we can keep in sync. I can't keep track of what you're doing with these force pushes.

@ilia-kats
Copy link
Contributor Author

Apologies, I didn't get an email for your PR and so I missed it. I merged it now, thanks.

@grimbough
Copy link
Owner

No problem. I'll wait until the CI check have passed, and then merge this master and push to Bioconductor.

That is, assuming the changes do what you want!

@ilia-kats
Copy link
Contributor Author

Great, thanks. My usecase for the moment is pretty much covered by the h5createAttribute/h5writeAttribute part, so yeah, it does what I want :)

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #101 (c7bb091) into master (87623b2) will increase coverage by 0.02%.
The diff coverage is 82.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   77.82%   77.85%   +0.02%     
==========================================
  Files          35       35              
  Lines        1876     1892      +16     
==========================================
+ Hits         1460     1473      +13     
- Misses        416      419       +3     
Impacted Files Coverage Δ
R/h5create.R 84.48% <55.55%> (-1.75%) ⬇️
R/H5T.R 91.89% <100.00%> (+3.00%) ⬆️
R/h5write.R 83.91% <100.00%> (+0.11%) ⬆️
R/h5writeAttr.R 90.24% <100.00%> (+2.00%) ⬆️

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 d7e1897...c7bb091. Read the comment docs.

@grimbough
Copy link
Owner

This has now been merged and should be available from Bioconductor in rhdf5 version 2.39.2 in the next day or so. Thanks a lot for the help.

@grimbough grimbough closed this Nov 29, 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.

None yet

2 participants