Allow continuation of verbosity level by mjsir911 · Pull Request #663 · PyCQA/pycodestyle

mjsir911 · 2017-07-09T23:17:06Z

I've been having to do pycodestyle.StyleGuide(verbose=True, paths=['-v']) to achieve verbosity to the point where I can see the configuration files included such as:

user configuration: /home/mjsir911/.config/pycodestyle
cli configuration: setup.cfg

This simply allows pycodestyle.StyleGuide(verbose=True) to display these verbosity messages.

IanLee1521

This looks fine to me overall.

One question is hat I'm a little unsure what your comment in line 2284 means... could you clarify?

sigmavirus24 · 2017-07-13T11:49:26Z

    (options, args) = parser.parse_args(arglist)
    options.reporter = None

+    if verbose:  # If specified verbose, continue on verbosity


I don't think this is necessary, nor do I think the addition to the signature on 2260 is necessary I'll explain in a different comment. This, however, does not help someone who is trying to pass verbose=False directly meaning that this feature is a bit half-baked.

Would you suggest alternatively having the keyword argument default to verbose=None, and just check like this?

if verbose is not None: options.verbose = verbose

sigmavirus24 · 2017-07-13T11:51:34Z

        # build options from dict
        options_dict = dict(*args, **kwargs)
        arglist = None if parse_argv else options_dict.get('paths', None)
+        verbose = options_dict.get('verbose', False)


So we allow verbose to be in the options dictionary, yes? If it is, it was passed here explicitly. Otherwise, (in the case where someone isn't directly using the StyleGuide but is instead using the CLI) we'll parse it from the arguments. Below here, we do options.__dict__.update(options_dict) which means that after we've processed options, we update the values unconditionally so long as options_dict isn't an empty dictionary. That's all you should need.

I'm a bit confused, what is the change you are suggesting I make?

So, in your use case, you are going to do:

StyleGuide(verbose=True)

Which means that options_dict will look at least like {'verbose': True}. After we call process_options on L1999,L2000 we say if options_dict: options.__dict__.update(options_dict) which means that regardless of what's passed on the CLI we will set verbose to True. That means that you don't need to modify process_options.

I suspect the problem you're actually encountering and trying to solve by this PR is that everything in pycodestyle expects verbose to be an integer. See, as one example, this line:

pycodestyle/pycodestyle.py

Line 555 in 19e85d9

if verbose >= 3:

If you're not seeing the output you expect when using the Python API, it's because True is being treated as 1 by Python and you're only getting basic verbosity levels.

~ ❯❯❯ python -c 'print(True + 1)' 2 ~ ❯❯❯ python3 -c 'print(True + 1)' 2 ~ ❯❯❯ python -c 'print(True >= 1)' True ~ ❯❯❯ python -c 'print(True >= 2)' False

I did not know that the verbose is meant to be a number, so I've changed that accordingly.

I do need to modify process_options somehow due to the fact that process_options calls read_config

pycodestyle/pycodestyle.py

Line 2343 in 19e85d9

options = read_config(options, args, arglist, parser)

And read_config checks for options.verbose

pycodestyle/pycodestyle.py

Lines 2251 to 2252 in 19e85d9

if options.verbose:

print('user configuration: %s' % USER_CONFIG)

Which does not get set until L1999 & L2000, after read_config is called, unless you instantiate StyleGuide(paths=['-v']), which does set options.verbose to 1.

I do not think it makes sense to have to pass a CLI argument to the styleguide object to achieve full verbosity, as that is what the verbose argument is for.

Sorry, I think you had mentioned this before. Thank you for reminding me.

So then the changes necessary here are the following:

verbose = options_dict.pop('verbose', 0)

Then below pass it to process_options as you are. You can make the change there of having verbose=None in the argument list and only if verbose is not None will you set it on the options object.

I think a better path forward, however, is to refactor pycodestyle and remove config reading from CLI argument processing but that's not for this PR.

sigmavirus24 · 2017-09-27T11:52:37Z

Hi @mjsir911

Do you have any intentions to fix this up?

mjsir911 · 2017-10-21T23:37:48Z

Sorry I haven't touched this PR, will start addressing concerns.

sigmavirus24

Let me know if you have any questions. I think you're 90% of the way to a correct solution here.

sigmavirus24 · 2017-10-22T14:05:30Z

        # build options from dict
        options_dict = dict(*args, **kwargs)
        arglist = None if parse_argv else options_dict.get('paths', None)
+        verbose = options_dict.get('verbose', False)


Sorry, I think you had mentioned this before. Thank you for reminding me.

So then the changes necessary here are the following:

verbose = options_dict.pop('verbose', 0)

Then below pass it to process_options as you are. You can make the change there of having verbose=None in the argument list and only if verbose is not None will you set it on the options object.

I think a better path forward, however, is to refactor pycodestyle and remove config reading from CLI argument processing but that's not for this PR.

sigmavirus24

One last item.

sigmavirus24 · 2017-10-22T14:25:02Z

        options_dict = dict(*args, **kwargs)
        arglist = None if parse_argv else options_dict.get('paths', None)
-        verbose = options_dict.get('verbose', False)
+        verbose = options_dict.get('verbose', 0)


Actually, I just realized, having the default of 0 here means we always override options.verbose to 0 even when it's not used via the API. So we can just do options_dict.pop('verbose', None). And yes, we want pop.

pop will not work because at the end of read_config, options is overwritten with command line arguments, which means if -v was not passed somehow it would default to 0.

pycodestyle/pycodestyle.py

Lines 2303 to 2304 in 6faef2b

# Third, overwrite with the command-line options

(options, __) = parser.parse_args(arglist, values=new_options)

This problem has not been apparent because as you pointed out earlier, options_dict gets copied to options.__dict__, but if I pop the verbose key, will not get copied.

Alternatively, (options, __) = parser.parse_args(arglist, values=options) seems to work, but I'm not sure why the previous values=new_options was done the way it was.

sigmavirus24 · 2017-10-23T11:32:19Z

        options_dict = dict(*args, **kwargs)
        arglist = None if parse_argv else options_dict.get('paths', None)
-        verbose = options_dict.get('verbose', 0)
+        verbose = options_dict.get('verbose', None)


Please use dict.pop.

pop will not work because at the end of read_config, options is overwritten with command line arguments, which means if -v was not passed somehow it would default to 0.

pycodestyle/pycodestyle.py

Lines 2303 to 2304 in 6faef2b

# Third, overwrite with the command-line options

(options, __) = parser.parse_args(arglist, values=new_options)

This problem has not been apparent because as you pointed out earlier, options_dict gets copied to options.__dict__, but if I pop the verbose key, will not get copied.

Alternatively, (options, __) = parser.parse_args(arglist, values=options) seems to work, but I'm not sure why the previous values=new_options was done the way it was.

Thanks for pointing that out. I missed that.

So if you look at the loop that builds up new_options it's pulling in values from config files. But in the chain of preferences, we always want CLI arguments to override those. This is why we one last time do parser.parse_args(arglist, values=new_options). We want to default to what's in the config file, and override those values with what's provided on the CLI.

- Forward down verbosity level

c556daa

IanLee1521 reviewed Jul 13, 2017

View reviewed changes

sigmavirus24 requested changes Jul 13, 2017

View reviewed changes

mjsir911 added 2 commits October 22, 2017 09:59

Transform booleans to numbers to go with previous code

b0cdd43

Handle conflicting CLI and verbose=# better

80fcdae

sigmavirus24 requested changes Oct 22, 2017

View reviewed changes

Default verbose to None instead of 0

d436d95

sigmavirus24 requested changes Oct 23, 2017

View reviewed changes

sigmavirus24 approved these changes Nov 18, 2017

View reviewed changes

sigmavirus24 merged commit c631809 into PyCQA:master Nov 18, 2017

	if options.verbose:
	print('user configuration: %s' % USER_CONFIG)

	# Third, overwrite with the command-line options
	(options, __) = parser.parse_args(arglist, values=new_options)

Uh oh!

Conversation

mjsir911 commented Jul 9, 2017

Uh oh!

IanLee1521 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsir911 Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigmavirus24 commented Sep 27, 2017

Uh oh!

mjsir911 commented Oct 21, 2017

Uh oh!

sigmavirus24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigmavirus24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjsir911 Oct 22, 2017 •

edited

Loading