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

Fix duplicate anchors #57

Merged
merged 3 commits into from
Jan 24, 2019
Merged

Fix duplicate anchors #57

merged 3 commits into from
Jan 24, 2019

Conversation

bedapisl
Copy link
Contributor

@bedapisl bedapisl commented Jan 23, 2019

Filip: The example in https://github.com/Cognexa/cxflow/pull/222 works for me now, so this code should work too.

@coveralls
Copy link

coveralls commented Jan 23, 2019

Pull Request Test Coverage Report for Build 1250

  • 5 of 6 (83.33%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 87.89%

Changes Missing Coverage Covered Lines Changed/Added Lines %
emloop/datasets/base_dataset.py 1 2 50.0%
Totals Coverage Status
Change from base Build 1199: -0.02%
Covered Lines: 3919
Relevant Lines: 4459

💛 - Coveralls

@bedapisl bedapisl requested a review from FloopCZ January 23, 2019 13:46
@@ -52,7 +52,7 @@ def make_simple(data: Any) -> Any:
:param data: data to be made simple (dict instead of CommentedMap etc.)
:return: simplified data
"""
return yaml.load(yaml.dump(data, Dumper=ruamel.yaml.RoundTripDumper), Loader=ruamel.yaml.Loader)
return dict(data)
Copy link
Member

Choose a reason for hiding this comment

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

if this works (not sure), than the whole make_simple is useless and dict should be used instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. @bedapisl claims that the original issue with dict cast is fixed.

Copy link
Contributor

@FloopCZ FloopCZ left a comment

Choose a reason for hiding this comment

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

Thank you @bedapisl !

@FloopCZ FloopCZ merged commit b55f932 into dev Jan 24, 2019
@FloopCZ FloopCZ deleted the yaml branch January 24, 2019 10:23
@blazekadam
Copy link
Contributor

Well... this will not convert anything nested right?...

@bedapisl
Copy link
Contributor Author

bedapisl commented Jan 28, 2019

I don't know what exactly you mean, but the class we are converting is nested in a way (it represents nested yaml configuration) and it looks like it is converted correctly.

@FloopCZ
Copy link
Contributor

FloopCZ commented Jan 28, 2019

I actually don't remember why we need the conversion whatsoever. Maybe we can skip the dict call entirely.

@FloopCZ FloopCZ mentioned this pull request Feb 11, 2019
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.

5 participants