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

[hail] Fix `hadoop_ls` to glob #7611

Merged
merged 4 commits into from Dec 2, 2019
Merged

[hail] Fix `hadoop_ls` to glob #7611

merged 4 commits into from Dec 2, 2019

Conversation

@tpoterba
Copy link
Collaborator

tpoterba commented Nov 25, 2019

No description provided.

@konradjk

This comment has been minimized.

Copy link
Collaborator

konradjk commented Nov 26, 2019

Worth adding a test?

@akotlar

This comment has been minimized.

Copy link
Collaborator

akotlar commented Nov 26, 2019

@konradjk Python tests exist

https://github.com/hail-is/hail/blob/master/hail/python/test/hail/utils/test_utils.py#L114

r = Env.jutils().ls(path, Env.hc()._jhc)

edit: GitHub is fancy

@konradjk

This comment has been minimized.

Copy link
Collaborator

konradjk commented Nov 26, 2019

One that uses a glob though! I don’t see one at least. Maybe I missed (though I can’t imagine since it should have failed that test before this)

@akotlar

This comment has been minimized.

Copy link
Collaborator

akotlar commented Nov 26, 2019

You’re right, it could be useful to have a scala-specific test, but any breaking changes away from old ls behavior will cause the hadoop_ls test to fail (since Python presents our public interface, the more important single test to have)

edit: iPhone typos

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

tpoterba commented Nov 26, 2019

yeah, will add a test for the new functionality though

@konradjk

This comment has been minimized.

Copy link
Collaborator

konradjk commented Nov 26, 2019

Out of curiosity, what's different about assert and the self.assertEqual?

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

tpoterba commented Nov 27, 2019

self.assertEqual is unittest style. We use pytest now, which rewrites Python assert statements to provide good failure messages. New code should use assert, I think.

@johnc1231

This comment has been minimized.

Copy link
Contributor

johnc1231 commented Dec 2, 2019

Somehow block matrix tests seem to be failing on this PR, both for the doctests and for the regular python tests.

tpoterba added 2 commits Dec 2, 2019
fix
@danking danking merged commit 8d912a1 into hail-is:master Dec 2, 2019
1 check passed
1 check passed
ci-test success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.