Skip to content

Conversation

@marqh
Copy link
Member

@marqh marqh commented Oct 5, 2016

No description provided.

.travis.yml Outdated
script:
- if [[ $TEST_TARGET == 'default' ]]; then
python -m iris.tests.runner --default-tests --system-tests --print-failed-images;
python -m iris.tests.runner --default-tests --system-tests;
Copy link
Member

Choose a reason for hiding this comment

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

I think it could make sense to remove the support for the "print-failed-images" key from setup.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

for now i'm putting this back in; if it is removed then we can discuss removing the functionality too

else:
# Cherry-pick the registered expected hashes from the
# test case uri/s.
expected = []
Copy link
Member

@pp-mo pp-mo Oct 5, 2016

Choose a reason for hiding this comment

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

I think a list comprehension would be clearer than init + append here.
Seems an obvious simplification of this code.

# test case uri/s.
expected = []
for uri in uris:
ehash = os.path.splitext(os.path.basename(uri))[0]
Copy link
Member

Choose a reason for hiding this comment

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

I'd still prefer that the url + hash were kept logically separate, and not insist that the filename "is" the hash.
The decision to do that may be enforced elsewhere, but better not to assume / require it here ?

@marqh marqh mentioned this pull request Oct 5, 2016
@marqh
Copy link
Member Author

marqh commented Oct 5, 2016

replaced by #2162

@QuLogic
Copy link
Member

QuLogic commented Oct 6, 2016

It wasn't really necessary to open a new PR; just force push on this one.

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.

4 participants