Fix macro hygiene bug, remove unnecessary compat macros, and deprecated array constructor #354
Conversation
|
For https://github.com/JuliaIO/HDF5.jl/blob/master/src/plain.jl#L2378 |
|
|
|
Actually https://github.com/JuliaIO/HDF5.jl/blob/master/src/plain.jl#L2378 should probably be a |
| @@ -149,6 +151,8 @@ ucoder = read(fr, "ucode") | |||
| @assert ucode == ucoder | |||
| salut_splitr = read(fr, "salut_split") | |||
| @assert salut_splitr == salut_split | |||
| salut_splitr = read(fr, "salut_2d") | |||
| @assert salut_2d == salut_2d | |||
There was a problem hiding this comment.
It's the same as every other test in this file. I didn't want to change the spirit of the testing structure.
There was a problem hiding this comment.
The testing infrastructure could use a revamp. Perhaps best left to a different PR.
There was a problem hiding this comment.
no, you're testing a == a here, the lhs and rhs should be different variables for this to mean anything (other than maybe a nan check)
There was a problem hiding this comment.
it's supposed to be salut_2d == salut_splitr it's a typo
|
Amazing os x now builds hdf5. not sure what changed, perhaps something on master to fix the issue. |
|
does JLD make use of any of the PROPERTIES consts? |
|
? |
| @@ -2145,19 +2145,19 @@ function h5a_get_name(attr_id::Hid) | |||
| len = h5a_get_name(attr_id, 0, C_NULL) # order of args differs from {f,i}_get_name | |||
| buf = Array(UInt8, len+1) | |||
| h5a_get_name(attr_id, len+1, buf) | |||
| @compat unsafe_string(buf[1:len]) | |||
There was a problem hiding this comment.
a bug. doesn't make sense to unsafe_string an UInt8 array, we want the name
musm
changed the title
|
Anything else I need to take care of? After merging this (when ready) can we please tag a release. Log files on travis are ridiculously long now.... |
|
bump |
|
are there any active maintainers here other than Tony, a lot of trivial PRs and clean up tasks remain but have not received any attention in months? |
|
please don't move code around in the same PR as you make deprecation changes, that's needlessly difficult to review |
|
You can see the individual commit history for the changes. |
|
It's better not to squash in this case |
|
Still requires actually verifying that you didn't change any of the moved code. That commit is not necessary. |
|
You can have a little trust in me ;) |
|
No, I do not, that's the point. If you want to rearrange code in a way that github can show as a simple code movement it can be done in a different PR. |
|
I should perhaps point out that @tkelman generally doesn't trust anyone – including me (nor should he). I can attest first hand that this can be very frustrating, but making absolutely sure that things aren't broken rather than just taking it on faith is why @tkelman is such a good release manager and valuable but tough code reviewer. Please don't take it personally or the wrong way! |
|
Thanks for taking out the reorg commit. This was basically ready to go without it. Win32 on 0.4 was a fluke download failure, but it passed on win64 0.4 and win32 0.5 so I'm merging. |
No description provided.