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

Fixed #485 #494

Merged
merged 10 commits into from Jun 29, 2018

Conversation

Projects
None yet
3 participants
@henhuy
Copy link
Contributor

henhuy commented May 28, 2018

scalars and sequences of param_results are now pandas objects

Function outputlib.views.node now also works with param_results.

Related issue: #485

@henhuy henhuy requested a review from uvchik May 28, 2018

@henhuy henhuy self-assigned this May 28, 2018

@henhuy henhuy added this to the v0.3.0 milestone May 28, 2018

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented May 28, 2018

This fix changes the API as output changes

@uvchik uvchik changed the base branch from dev to dev03 May 28, 2018

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 28, 2018

I changed the base to dev03, where we collect the API-changes.

@uvchik

uvchik approved these changes May 28, 2018

Copy link
Member

uvchik left a comment

For me it looks fine. We can merge it into dev03 and discuss about my other suggestions in issue #475 and/or PR #463

Fix `.travis.yml`
Although `datapackage` is an optional dependency, the module using it
still gets run by `nosetests --with-doctest` since that tries to load
every module in order to discover doctests. That's why the travis
configuration has to be changed to install the optional dependencies
too, when installing the package to test it.
@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 28, 2018

I fixed the tests caused by another PR but there is a Python-3.6-issue. It seems to me that the order of the keys is different in Python 3.6 and therefore the assert test fails.

gnn added some commits May 28, 2018

Squelch deprecation warnings
Wrapping regular expressions in raw strings using `r''` is a good idea
anyways, as that prevents stuff like `\s` from being interpreted as a
string escape sequence instead of a regular expression character class.
Unless, of course, you want `\n` to be interpreted as a newline. Then
you have to use regular strings.
The joys of overlapping syntaxes...
Fix Python 3.6 test errors
As of Python 3.6, dictionaries are now [order preserving][0]. This made
some tests fail. I fixed them by ordering the dictionaries in the test
cases appropriately. I just have to hope, that the order generated in
the actual code is predictable and the tests don't fail in other
environments.

[0]: https://docs.python.org/3.6/whatsnew/3.6.html#new-dict-implementation

@gnn gnn force-pushed the fixes/#485 branch from 2ff5d29 to 8253a9d May 28, 2018

@gnn

This comment has been minimized.

Copy link
Member

gnn commented May 28, 2018

Yeah. Starting with Python 3.6, dictionaries preserve insertion order.
Fixed the tests. Hope this doesn't come up again or we have to find a more sustainable solution.

Merge dev into fixes/#485
Update the branch to current developments in order to fix merge
conflicts.
@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 28, 2018

Thank you for your fixes. Shall we merge this one into dev03 and go on with PR #485.

@gnn

This comment has been minimized.

Copy link
Member

gnn commented May 28, 2018

Yeah, sure.

Also, something totally unrelated: here's a short and a slightly longer guide to commit messages. The two points why I bring that up are these:

  • Capitalized, short (50 chars or less) summary: the first letter should be capitalized because the first line of a commit might be used as a subject line in an email and those are usually capitalized to make them easily distinguishable from a leading Re: or a [mailing list] prefix.
  • Write your commit message in the imperative...: There are two reasons for this. First: it lines up with how Git phrases its automatically generated commit messages. Second: a subject line written in imperative completes a sentence starting with: "If you apply this commit, it will ...".

It's nothing important really, but I thought now's as good a time to point this out as any. And yeah. I should probably put those links in "How to contribute" or somewhere similar.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented May 28, 2018

And yeah. I should probably put those links in "How to contribute" or somewhere similar.

We already have the second link in our dev-rules:
http://oemof.readthedocs.io/en/stable/developing_oemof.html#commit-message

@henhuy henhuy referenced this pull request May 28, 2018

Merged

Revise param_results/results (e.g. #463) #485

6 of 6 tasks complete

@uvchik uvchik modified the milestones: v0.3.0, v0.2.2 Jun 11, 2018

@uvchik uvchik changed the base branch from dev03 to dev Jun 29, 2018

@uvchik uvchik merged commit 1ba0dd3 into dev Jun 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on fixes/#485 at 80.134%
Details

@uvchik uvchik deleted the fixes/#485 branch Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment