Skip to content

refactor main.py, flesh out functions from main function#1059

Merged
boegel merged 16 commits intoeasybuilders:developfrom
boegel:cleanup_main
Oct 16, 2014
Merged

refactor main.py, flesh out functions from main function#1059
boegel merged 16 commits intoeasybuilders:developfrom
boegel:cleanup_main

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Oct 13, 2014

@stdweird, @wpoely86: please review?

The diff is going to look a bit messy, maybe you want to look at the actual new main.py instead...

@hpcugentbot
Copy link
Copy Markdown

Test PASSed.

Comment thread easybuild/main.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move to easyconfig.tweak?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

well, this also relates to --from-pr, which has nothing to do with --try-X or tweak.py... But I'll find a better location for this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

moved to easyconfig.tools, since get_paths_for also lives there

@stdweird
Copy link
Copy Markdown
Contributor

a lot of the functions should be moved to modules. an dnew functions imply new unittests imho.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 14, 2014

@stdweird: remarks fixed; w.r.t. unit tests: most of main.py is already covered by the unit tests, especially the ones in test/framework/options.py, so there's no need for more imho

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 14, 2014

Comment thread easybuild/framework/easyconfig/tools.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove reference to robot

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

@hpcugentbot
Copy link
Copy Markdown

Test PASSed.

Comment thread easybuild/main.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add import to previous line

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

Comment thread easybuild/tools/robot.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just if candidates:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

o
k

@stdweird
Copy link
Copy Markdown
Contributor

reread completed

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 15, 2014

@stdweird: remarks handled, please rererererererererereview

@hpcugentbot
Copy link
Copy Markdown

Test FAILed.

Comment thread easybuild/framework/easyconfig/tools.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use

kwargs={}
if try_to_generate:
    kwargs["build_specs"]=build_specs
ecs = process_easyconfig(ec_file, **kwargs)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you mean not if try_to_generate, but ok, fixed

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 15, 2014

@stdweird: fixed remark and broken unit test, not sure if you were done rererererererererereviewing?

@hpcugentbot
Copy link
Copy Markdown

Test FAILed.

@stdweird
Copy link
Copy Markdown
Contributor

@boegel LGTM, but tests fail, so now it's up to @wpoely86 to make the next batch of comments ;)

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 15, 2014

@stdweird: a test is failing that never should've worked, I'll figure it out

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 15, 2014

fixed totally broken test, which went undetected because of using print_error which is now replaced by _log.error...

@hpcugentbot
Copy link
Copy Markdown

Test PASSed.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 16, 2014

@wpoely86 will be afk for a while, so I'll merge this in as is, as discussed with @stdweird

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.

3 participants