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

Look for DOIT_CONFIG in config so it can be set in conf.py #1716

Merged
merged 7 commits into from May 12, 2015

Conversation

@bnmnetp
Copy link
Contributor

bnmnetp commented May 12, 2015

Addresses feature request in #1715

if 'DOIT_CONFIG' in config:
DOIT_CONFIG = config['DOIT_CONFIG']
else:
DOIT_CONFIG = {}

This comment has been minimized.

Copy link
@ralsina

ralsina May 12, 2015

Member

It's easier to just do DOIT_CONFIG = config.get('DOIT_CONFIG', {})

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

Indeed it is. I have this habit of writing all of my code like I'm preparing to show it in my CS1 course.

bnmnetp added 2 commits May 12, 2015
@@ -62,6 +62,7 @@


def main(args=None):

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

Don’t put empty lines at the beginnings of functions.

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

Removed. I guess I didn't clean up properly after removing some debug prints.

'reporter': ExecutedOnlyReporter,
'outfile': sys.stderr,
}
DOIT_CONFIG['reporter'] = ExecutedOnlyReporter

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

Wouldn’t it be nicer to let users’ DOIT_CONFIG override this? Some people might want to override our opinionated reporter choice.

if self.quiet:
    DOIT_CONFIG = {…}  # from original code
else:
    DOIT_CONFIG = {…}

DOIT_CONFIG.update(config.get('DOIT_CONFIG', {}))

We also need to pop DOIT_CONFIG out of the config dict in nikola.py (we don’t need to save it anywhere though) so self.configured works correctly.

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

updating the dictionary as you suggest makes sense to me. I hadn't thought that you guys would want anyone to override what you had coded in there.

Since DOIT_CONFIG may or may not be in config. Why don't I just check and pop it out of config in the load_tasks function?

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

So, it seems like going back to the original and adding this one line:

        DOIT_CONFIG.update(config.pop('DOIT_CONFIG', {}))

would do what you want.

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

load_tasks is called after configuration is passed to the site object. We can’t do that.

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

So then we can't just pop it out and throw it away in __init__ It has to stay somewhere so that load_tasks can get to it.

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

Then pop it out in Nikola.__init__ and use nikola._doit_config here. (besides, where does config come from in this code?)

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

config is declared as a global on line 58 of __main__ and then populated on line 114 in main after loading conf.py

        config = conf.__dict__

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

ah, a global. That’s evil.

Still, you can just get it from self.nikola if you pop it out there.

This comment has been minimized.

Copy link
@bnmnetp

bnmnetp May 12, 2015

Author Contributor

I tried:

AttributeError: 'Nikola' object has no attribute '_doit_config'

I'm guessing you were suggesting I create _doit_config

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

Yes, that’s what I was suggesting. Add after nikola.py line 287:

        self._doit_config = config.pop('DOIT_CONFIG', {})
@schettino72
Copy link
Member

schettino72 commented May 12, 2015

I guess normal Nikola users might not know about doit at all. So maybe better expose it as a different name than DOIT_CONFIG.

@Kwpolska
Copy link
Member

Kwpolska commented May 12, 2015

“Normal Nikola users” should not know about this setting.

@schettino72
Copy link
Member

schettino72 commented May 12, 2015

“Normal Nikola users” should not know about this setting.

why not? This allow users to change default from command line options.
@bnmnetp himself wants to change... you also mention about changing the reporter...

@bnmnetp
Copy link
Contributor Author

bnmnetp commented May 12, 2015

configuring a value for a command line option certainly felt to me like a thing "normal" users may want to do.
@schettino72 are you suggesting that in conf.py we expose this under a different name than DOIT_CONFIG? maybe CL_PARAMETERS or something more descriptive? Although I have no idea how many of the command line options exposed are configurable through doit and how many are not.

@Kwpolska
Copy link
Member

Kwpolska commented May 12, 2015

Is DOIT_CONFIG even documented by doit itself?

@bnmnetp
Copy link
Contributor Author

bnmnetp commented May 12, 2015

I found it here: http://pydoit.org/cmd_run.html

I don't spend my whole day looking through source :-)

@@ -241,14 +241,15 @@ def load_tasks(self, cmd, opt_values, pos_args):
if self.quiet:
DOIT_CONFIG = {
'verbosity': 0,
'reporter': 'zero',
'reporter': 'zero'

This comment has been minimized.

Copy link
@ralsina

ralsina May 12, 2015

Member

No need to change it, but I tend to leave the last commas in this sort of things because it makes the diff better the next time something is added :-)

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska May 12, 2015

Member

(It also lets the next person just type in their changes without having to care about adding a comma.)

@ralsina
Copy link
Member

ralsina commented May 12, 2015

It looks good enough for me. Add yourself to AUTHORS?

@schettino72
Copy link
Member

schettino72 commented May 12, 2015

@Kwpolska it is documented here: http://pydoit.org/configuration.html#configuration-at-dodo-py

@bnmnetp every command line option can be configured (and also some options that are not available through the command line)

ralsina added a commit that referenced this pull request May 12, 2015
Look for DOIT_CONFIG in config so it can be set in conf.py (Fix #1715)
@ralsina ralsina merged commit b0bf8de into getnikola:master May 12, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.