search

vtsuperdarn / davitpy

DEPRECATED The DaViT Python project
http://vtsuperdarn.github.com/davitpy/
GNU General Public License v3.0
37 stars 59 forks source link

Use absolute paths instead of relative #326

Closed asreimer closed 6 years ago

asreimer commented 7 years ago

This pull request fixes the bug reported in #309.

The problem was caused because relative paths were used instead of absolute paths. See https://github.com/vtsuperdarn/davitpy/issues/309#issuecomment-336820639 for more details.

Testing is: 1) Without this pull request, try importing davitpy from within the davitpy source directory (if you have the source at /home/user/davitpy, then cd there and try to import). This will fail. 2) With this pull request, try 1). It will work.

Thanks to @arfogg for reporting.

scivision commented 7 years ago

This seems fine just from looking. In the future, use of Python standard library pathlib (actually pathlib2 for py27 compatibility) will cleanup these long path code lines and remove ambiguity because path will be an object instead of just a string. Effectively zero time penalty for using pathlib.Path vs. str.

Reference using pathlib vs str

asreimer commented 7 years ago

That is a nice writeup @scivision. Thanks for sharing!

aburrell commented 6 years ago

Since we didn't want to merge this before because of the py3 pull request that is being re-done, I say we merge this in now. Thoughts @asreimer ?

ksterne commented 6 years ago

This code looks good and the test works out. It's likely it will be updated, but until then, this would be good to get the fix in. Can someone else give it a test and merge it it?

aburrell commented 6 years ago

You're the third person to test it @ksterne , so I think it's ok to merge now. Shall I do the honours?

ksterne commented 6 years ago

Oh, I didn't know others had actually tested it. I thought you'd just made a comment we could move forward since python3 is being separated out. I didn't think there was a test done by @scivision, but again, maybe just didn't read it right. Don't know why we would hold off here.

aburrell commented 6 years ago

I was thinking of you, me, and @asreimer , but my counting is wrong since @asreimer as the creator cannot test :-P

ksterne commented 6 years ago

Yea, self testing shouldn't quite count (as they should have it working before making the PR). It wasn't clear that you were good with it @aburrell, but if you're good then go ahead and merge it. This fix stems from a problem that I think you were associated or partially observed in #309.

aburrell commented 6 years ago

Mmmm… yes, I’ll test again with better comments so that our protocol remains satisfied and if it’s successful, merge. Sorry for my sloppy comments on this pull request.

On 1 Mar 2018, at 08:46, Kevin Sterne notifications@github.com wrote:

Yea, self testing shouldn't quite count (as they should have it working before making the PR). It wasn't clear that you were good with it @aburrell https://github.com/aburrell, but if you're good then go ahead and merge it. This fix stems from a problem that I think you were associated or partially observed in #309 https://github.com/vtsuperdarn/davitpy/issues/309.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vtsuperdarn/davitpy/pull/326#issuecomment-369614066, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuC_hV9f-lCiaKZPlqGHVXThq_xl1Hrks5taAmrgaJpZM4P6R_W.

ksterne commented 6 years ago

Eh, not a problem, it's not like it's a lot of code to be changed. Just couldn't quite tell.

aburrell commented 6 years ago

Alright, just tested again and it still works after the fix, but not before. Merging.