Skip to content

Conversation

@come-maiz
Copy link
Owner

No description provided.

@come-maiz
Copy link
Owner Author

Requires #2.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.453% when pulling 31a8f0a on verbosity1 into 937a5a0 on v1.

@fgimenez
Copy link

Looks good, I don't like too much the use of the literals 1 and 2 for the verbosity level instead of constants.

IMO it would be better to define a type as an alias of uint8 (for example, VerbosityLevel), then declare constants for the two levels with iota.

If we add more levels in the future we'll need to change the code anyway to take into account the new numbers and all the functions will be clearer when comparing the verbosity variable and constants with meaningful names, what do you think?

@come-maiz
Copy link
Owner Author

yes, I'll do that when I'm back. How would you name the levels?

@fgimenez
Copy link

We have three of them, beginning with 0, right? No idea, in fact it's a bit hard to find good names. I'm not sure if they are related to each other as incremental, for 1 and 2 they seem to add different things, maybe each comparison should be done with an equal instead of greater/lower than... Anyway, I think that any name will do, we can come back to this later.

@come-maiz
Copy link
Owner Author

I will take your last suggestion and report it as a bug. The last branch of this set plays a little with levels, and it will be easier if we land it before making this change. I'll do it first thing after the other branch. I propose QUIET, LOW and HIGH

@come-maiz come-maiz merged commit 4dee14e into v1 May 27, 2016
@come-maiz come-maiz deleted the verbosity1 branch May 27, 2016 00:59
@fgimenez
Copy link

Sorry for not being clear :) I was suggesting to use whatever name in the constants to replace the literals and come back to it later.

It would be also good to use names that don't imply increasing verbosity because it seems that, by merging the two separate variables into one, the intention of both are merged too, and for instance according to the unit tests level 0 doesn't print anything, level 1 prints the most and level 2 is in the middle, please correct me if I'm wrong. Anyway, we can handle it while addressing the issue.

@come-maiz
Copy link
Owner Author

hum, I didn't notice that the tests need to be renamed.
And no, 2 should be the most verbose one. 1 should be like -v, and 2 like -vv. Maybe there's something not so clear in the tests, the rename will make it better.

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.

4 participants