Improve section/order handling #89
Conversation
| @@ -116,6 +116,18 @@ def __init__(self, section): | |||
| 'Section {} never defined.'.format(section)) | |||
|
|
|||
|
|
|||
| class DuplicateSection(BadStructure): | |||
| def __init__(self, section): | |||
| super(DuplicateSection, self).__init__( | |||
There was a problem hiding this comment.
nit: since (IIRC) Vimdoc now requires Python 3, this can just be super().__init__(.
There was a problem hiding this comment.
Not worth going out of our way to use the new convention, though.
|
Mostly looks reasonable, but see above. |
| @@ -60,11 +60,17 @@ def Merge(self, block, namespace=None): | |||
| # Overwrite existing section if it's a default. | |||
| if block_id not in self.sections or self.sections[block_id].IsDefault(): | |||
There was a problem hiding this comment.
This might be simpler if we just invert that check, unless there's some reason we need to be able to support overwriting a default section with another default section?
# Allow overwriting an existing section if it's a default.
if block_id in self.sections and not self.sections[block_id].IsDefault():
raise error.DuplicateSection(block_id)
self.sections[block_id] = block(Likewise just below.)
There was a problem hiding this comment.
I actually tried that at first, but there's a third case that's missing (default over non-default shouldn't throw error). Just played with a few variations on inverting the check again but didn't find any better version.
There was a problem hiding this comment.
I'm surprised we are trying to overwrite non-defaults with defaults, but if that's what we need to do, then this is certainly reasonable.
There was a problem hiding this comment.
Yeah, it's a matter of traversal order. Special flags like plugin[foo] can have a default from one file (plugin/foo.vim) and explicit value declared in another, so we don't know which definition will come last and try to "overwrite" the other.
|
LGTM whichever way you decide to go on the above, btw. |
|
BTW, I'm still updating and addressing the other comments. I think I do want to get the implicit order into the "natural order" that makes intuitive sense to me, but I'm still having trouble describing the algorithm in plain English. Anyway, I'm adding some tests for the ordering behavior since it's clearly non-trivial. =) |
|
PTAL. Improved the default ordering and added more tests for it. |
| # Add any undeclared sections before custom sections, except 'about' which | ||
| # comes at the end by default. | ||
| section_insertion_idx = 0 | ||
| order = order[:] |
There was a problem hiding this comment.
Any reason not to take a copy at the point of assignment above?
i.e.
order = explicit_order[:] or []There was a problem hiding this comment.
Yeah, I tried that version first, but it fails for None.
|
LGTM, thanks (and sorry for the delay). |
Improve section/order handling
Fix having to explicitly declare every built-in section when you use
@orderand complain if you have redundant definitions for the same section.Fixes #88.
Also adds a few initial tests to get the ball rolling on #29.