This introduces the new defaults format "commandname.default.optionname" which
directly overrides the default of the option, instead of prepending the
command-line option. This is meant to replace the [defaults] section which is
already deprecated in a manner that's easier and safer to use than creating
aliases.
Details
- Reviewers
yuja baymax - Group Reviewers
hg-reviewers
Diff Detail
- Repository
- rHG Mercurial
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
FYI this is a change I had previously sent to the list as 60b3222e01f96f91ece7eda9681a89bf3bb930a6, and Yuya reviewed . I just had never followed up on it.
mercurial/dispatch.py | ||
---|---|---|
624 | Maybe this type conversion can be a fancyopt.customopt method since we've # no idea if _defaultopt() should be made public or the whole commands.default handling # should be moved to fancyopts x = fancyopts._defaultopt(olddefault) newdefault = x.configdefault(ui, cmd, optname, ...) @dploch, any suggestions? | |
mercurial/ui.py | ||
390 ↗ | (On Diff #6523) | Perhaps this is noop since [commands] is removed at all if ui.plain() returns True. |
mercurial/dispatch.py | ||
---|---|---|
624 | The issue is that customopt (and all its children) assume the value type is already the correct one, and thus do not perform any conversion. Since we're parsing values from the config file, the conversion is desired to ensure they don't all end up as text - the config{bool,int,etc} methods called by configtyped perform the proper conversion. In most cases (all commands that declare default values) no conversio is needed since those already have the correct type. | |
mercurial/ui.py | ||
390 ↗ | (On Diff #6523) | You're right, I had the plain logic inverted in my head. Removed. |
mercurial/dispatch.py | ||
---|---|---|
624 | IIUC, an extension author may implement its own customopt subclasses, and |
mercurial/dispatch.py | ||
---|---|---|
624 | Ah, makes sense. See if this addresses that case satisfactorily - it still has the caveat of not being able to "reset" container types, but that's true of the command line as well (if you have a list flag with a non-empty default, there's no way to remove that default item). |
mercurial/dispatch.py | ||
---|---|---|
624 | I think rdamazio's response is correct. The type information for an opt exists only in the 'defaultvalue' slot in the tuple, so it needs to be extracted from there before reading the config. I don't have a strong opinion as to where this code should go - 'ui' does seem marginally more appropriate than 'fancyopts', since it keeps 'fancyopts' low-level and not depending on 'ui'. | |
625–626 | This doesn't handle callables properly. I wonder if the something like the following would work instead: oldopt = fancyopts._defaultopt(olddefault) Where '_withnewdefault' is a wrapper customopt that just changes the default. | |
639–640 | This makes me nervous. What if someone re-uses a customopt instance in multiple commands? i.e.: DATE_FLAG = mypkg.dateopt() Now, setting commands.defaults.before=2018-03-05 also silently changes the default for 'after'. I suspect we need to introduce a wrapper class like what I suggest on lines 625-625, that delegates and leaves the original default unchanged. And either way, we should probably clarify in the docs on customopts what expected use of the class is (i.e., should we just forbid reuse, is 'oldstate' safe to mutate, etc.) |
mercurial/dispatch.py | ||
---|---|---|
625–626 | I thought callables were meant to be used to generate the default default, not with overridden values? | |
639–640 | Nobody should use the same *instance* on multiple flags. Even with the current flags, if you use e.g. the same list on many, that'll cause problems with listopt. |
There seems to have been no activities on this Diff for the past 3 Months.
By policy, we are automatically moving it out of the need-review state.
Please, move it back to need-review without hesitation if this diff should still be discussed.
:baymax:need-review-idle:
Maybe this type conversion can be a fancyopt.customopt method since we've
refactored the default handling by D2090?
@dploch, any suggestions?