-
Notifications
You must be signed in to change notification settings - Fork 218
add support for --show-config (REVIEW) #1611
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
Conversation
|
Cool +1 for (optionally) always displaying that before the builds |
|
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2672/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
|
I like this, specially since I am working in an environment that I didn't set up. So +1 too. A couple of suggestion though:
|
|
@damianam: the full list of configuration options is going to be *long (and thus unreadable), see below. We should figure out a list of 'important' configuration options to always mention (even if they're default)? |
|
You have a good point there. What about having |
|
@damianam: support both makes sense, yeah I would still like to include a set of 'important' settings in the 'short' version though... Question is which ones. These? http://easybuild.readthedocs.org/en/latest/Configuration.html#mandatory-configuration-settings |
|
The options you propose to always show are a good start. My vote is:
The column with [F], [C], [E] should go on the left of the actual variable content. |
|
@gppezzi you mean like this? |
|
👍 Much better, or else that would float around in case of long paths, which is very annoying ;) |
|
reworked according to feedback, unit test add always included (even when default)
current output: |
|
EasyBuild framework unit test suite FAILed. See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2673/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
EasyBuild framework unit test suite FAILed. See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2674/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
EasyBuild framework unit test suite FAILed. See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2676/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
EasyBuild framework unit test suite FAILed. See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2679/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
EasyBuild framework unit test suite FAILed. See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2680/console for more details. Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do. |
|
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2681/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
|
@wpoely86: please review? |
|
documentation update via easybuilders/easybuild#200 |
| opts_dict[opt] = (opt_val, loc) | ||
|
|
||
| # determine max width or option names | ||
| nwopt = max([len(opt) for opt in opts_dict]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len of a tuble?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a no, ignore
easybuild/tools/options.py
Outdated
| sys.exit(0) | ||
|
|
||
| def pretty_print_opts(opts_dict): | ||
| """Pretty print options dict.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc arg
|
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2682/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
|
Going in, thanks for the feedback everyone! |
add support for --show-current-config (REVIEW)
Current output only shows values for configuration options if they're different from the default values.
I'm considering to always mention some key settings, regardless of whether they still have the default value or not, e.g. installpath, buildpath, sourcepath, module naming scheme, etc.
Also needs tests.