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

Add testability for 2.7 and make build no longer fails #10

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

francbartoli
Copy link
Collaborator

No description provided.

@darothen darothen reopened this Jul 17, 2017
@francbartoli
Copy link
Collaborator Author

build for py35 is still failing because the version module is breaking due to incompatibility with python 3

This line https://github.com/keleshev/version/blob/master/version.py#L2

should be replaced with

try:
    # Python 3
    from itertools import zip_longest
except ImportError:
    # Python 2
    from itertools import izip_longest as zip_longest

@darothen
Copy link
Owner

Yup. Mind adding one more commit with that quick fix? Then once it builds I'll merge.

@francbartoli
Copy link
Collaborator Author

Maybe...but we should make a PR against that repo if we want to automate the build on travis

@francbartoli
Copy link
Collaborator Author

francbartoli commented Jul 17, 2017

Close PR keleshev/version#7 since it's a duplicate of a PR already merged. Wait pypi updated to version 0.1.2

os.makedirs(full_path)
except OSError as e:
# if e.errno != errno.EEXIST:
print e
Copy link
Owner

Choose a reason for hiding this comment

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

Should change this print statement to the function form and add from __future__ import print_function to top of script

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah sorry I forgot to be still in a PR. Are you sure do we want my specific experiment make script on your library? Otherwise I should move to a different branch and maybe merge locally what I want to push on this PR

Copy link
Owner

Choose a reason for hiding this comment

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

That would be fine. I'm also open to saving this as an example of how the package can be used!

@@ -1,6 +1,6 @@
# experiment: Managing modeling experiment output

[![Build Status](https://travis-ci.org/darothen/experiment.svg?branch=master)](https://travis-ci.org/darothen/experiment)
[![Build Status](https://travis-ci.org/francbartoli/experiment.svg?branch=master)](https://travis-ci.org/francbartoli/experiment)
Copy link
Owner

Choose a reason for hiding this comment

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

For production let's leave this pointing to my repo :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, it was just to be sure if the build changed ;)

SEPARATOR = "_"
cases = [
Case("category", "Climate Data Category", ["Temperature Climate Data"]),
Case("variable", "Climate Variable", ["Temperature",
Copy link
Owner

Choose a reason for hiding this comment

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

This deserves an offline discussion. Usually when I use experiment, I don't include the variable or "field" as part of the path; I consider those output files to be the leaves of the file system/tree I'm trying to parse. I guess it becomes important if the field name is in the experiment hierarchy... but maybe that would be a worthy, separate change to the package to have an option to inject the field where it comes up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, let's move on skype to discuss it

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.

2 participants