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

Typecheck format2 #827

Merged
merged 26 commits into from Feb 7, 2014
Merged

Typecheck format2 #827

merged 26 commits into from Feb 7, 2014

Conversation

stdweird
Copy link
Contributor

(This sits on top of the build_stats PR)

@stdweird
Copy link
Contributor Author

@boegel i'm also tackling the suffix in version operator issue. i need matching separators.

@stdweird
Copy link
Contributor Author

@boegel this does not build (i need a vsc-base bump for that)

@stdweird
Copy link
Contributor Author

@boegel has Dependency class, sits on top of develop now

class Patches(ListOfStrings):
"""Handle patches as list of Patch
"""

Copy link
Member

Choose a reason for hiding this comment

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

add raise NotImplementedError?

@stdweird
Copy link
Contributor Author

@boegel small refactor and extra dealings with robot path

@boegel
Copy link
Member

boegel commented Jan 30, 2014

@stdweird: Merge with develop is broken (probably because of #828), can you look into fixing that?

@stdweird
Copy link
Contributor Author

@boegel done

# #

"""
This module implements easyconfig specific formats and their converts
Copy link
Member

Choose a reason for hiding this comment

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

s/converts/conversions/g


res = {}
ml_usage = [-1]
for idx, pairs in enumerate(self._split_string(txt, sep=self.separator_dict)):
Copy link
Member

Choose a reason for hiding this comment

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

why pairs? shouldn't that be entry or item or something like that? pairs doesn't make sense to me, but maybe I'm missing something

@stdweird
Copy link
Contributor Author

stdweird commented Feb 5, 2014

@boegel, i addressed most of the remarks. the rest i consider personal preferences. in particular wrt the tests, i only favour hardcoding if no class constants are used for the separators.

"""

res = {}
ml_usage = [-1]
Copy link
Member

Choose a reason for hiding this comment

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

add a comment here to explain why the list is initiated with -1? it looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the comment is below, but i'll try to come up with better implementation

@boegel
Copy link
Member

boegel commented Feb 5, 2014

@stdweird: One more missing license header in a test module.

We'll just need another PR to actually use Dependency where it's intended to be used. I'll create a placeholder PR right now, and then flesh it out tomorrow.

@stdweird
Copy link
Contributor Author

stdweird commented Feb 6, 2014

@boegel added the missing lic header

@boegel
Copy link
Member

boegel commented Feb 7, 2014

@stdweird: I'll merge this in, and create a new PR to actually start using Dependency. Including it in here will be too messy.

boegel added a commit that referenced this pull request Feb 7, 2014
@boegel boegel merged commit 845f471 into easybuilders:develop Feb 7, 2014
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.

None yet

2 participants