It appears like this was never tested.
Now that it is covered it's much easier to play around with the
implementation and compact it a bit.
Change-Id: Ie9cc14082f69e7240380d352fb362d0a3fa4d341
It's possible to specify a parameter with no name via TemplateData.
This confuses the template dialog because the empty string is a
reserved, internal value for parameters that are in the process of
being named.
Fixing TemplateData is not so easy. Therefor this workaround here.
See T333826 for a detailed description.
Bug: T333826
Change-Id: I5f4efd7069e71ba51138a23f3c3cb40e1b50d339
While at it, also fix a few broken uses of `@see` where `@link` was
intended or for full names, there is no syntax needed, as JSDuck
already links those.
Change-Id: Iaeb46b05c6f2e6f00198bc2ae773c895935b4cea
Such comments don't add any new information. The method signature
alone already tells the full story.
We did this already to a lot of the template related code we touched.
This is just a bit of cleanup to make it consistent.
Change-Id: I932b620910924a16dc0d31d6c8a3ab11818316fe
When I type e.g. "subst:example" as the template name, we made this
work as the user would expect: the template named "Example" is found
and it's TemplateData documentation used. But the dialog title shows
"Subst:example". Note the uppercase "S". It means this string is
parsed as a title, including the "subst:". This is confusing. Just
show the template name.
Change-Id: I9817786991a8379cf48b0a664aef1413abddee2d
The code in .cacheTemplateDataApiResponse() where the `specCache`
is filled skips missing pages. .setTemplateData() is never called.
While we could – in theory – check the `missing` flag (as done in
patchset 1), this flag never makes it to the spec.
Rather simple solution: Mark everything as undocumented, as long
as .setTemplateData() is not called.
This affects only missing pages. .setTemplateData() is called in
all other situations.
Bug: T272487
Bug: T276574
Bug: T290136
Change-Id: I7045e84f2f2ba5aa4591c94ea495b0249e6c40d6
There are 2 situations:
1. Either the template name is used in a [[…]] link. In this case
we must provide the namespace. MWTemplateModel.getTitle() does
this. However, it's not a mw.Title object and therefor not really
guaranteed to be a valid title. This is fine. The worst thing
that can happen is that the link points to an error message.
But this should be entirely unreachable anyway.
2. Some messages want to display the name of the template.
Ideally without the namespace. That's what
MWTemplateSpecModel.getLabel() is for. Again this is not
guaranteed to be a valid mw.Title. But it doesn't need to. It's
only used as a label.
Change-Id: I03d0481201620a2f5c444ee32b656bcaade98aac
Notably:
* Include parameter aliases, labels and descriptions in the
search.
* Don't use a possibly outdated search index, but live data.
* Clear filter when a new checkbox is added.
Bug: T272481
Change-Id: Ie90a803af6178a8bb6de370a0f8e079800d9f8a2
Separation of concerns:
* The template model knows which parameters are currently used,
but doesn't know what's documented.
* The spec knows what's documented, but doesn't know what's
currently used.
Change-Id: I97cac00d6775a17a07059d0e8a7a116adc6080b3
If a known parameter is present using one of it's aliases, then
only the aliased name should be shown to the user. This patch,
therefore, resolves the issue of the same parameter being added
to the sidebar twice.
When adding a parameter that is aliased, it will receive the same
position as the non-aliased parameter it is replacing.
Bug: T274545
Change-Id: If4e58c941fd0f0e690d3603935f5a5d3f9938163
This also removes a few lines of text that don't explain
anything that would not be obvious from the code or @return
tag anyway.
Change-Id: I2f8f02dd61c50d9990d72c0e8ea79d679c9b11f2
This fixes a minor issue in the spec class. In the first step,
parameters from the template are added to the list of known
parameters. Later, aliases are resolved. The original behavior
was that such a parameter moved to the end of the list. This
is rather unexpected.
This dosn't have much of an impact. The pretty much only place
where the parameter order from the spec can be seen is in the
parameter search widget. Still I believe it's worth fixing.
Bug: T285483
Change-Id: I455818451811e92bba3e9320c2d41e1db8d563f2
I don't want this code to crash when the TemplateData API
returns an unexpected result.
Bug: T285483
Change-Id: I237cbfbb85892a53a08d9e7e34cf4974775d627a
This doesn't "extend". It was never used like this. What it
actually does is to link between a (cached) TemplateData blob
and the spec class that want's to use it.
Is this the best possible name?
* fillFromTemplateData( … )?
* propagateTemplateDocumentation( … )?
* readDocumentationFrom( templateData )?
* …?
Do we want to rename the "spec" class as well?
* MWTemplateDocumentation?
* MWTemplateMetadata?
* MWTemplateDataAccessor?
* …?
Bug: T285483
Change-Id: I6c52ef42d411c2f47fc0080768d36ebda4dd2a55
Just store the JSON blob from the TemplateData API as is.
This comes with a bunch of nice consequences:
* Less code.
* Less class properties that don't do anything but copy what's
in the TemplateData blob.
* Easier to understand what's going on. The `this.templateData`
property is now a reference to the *actual* TemplateData
documentation.
* No need to cache the documentedParamOrder. Just do it when
needed.
This also removes an unused feature from the `extend()` method
that didn't made sense anyway. Before it was possible to merge
conflicting documentations. But this is not only unused, it's
impossible to have multiple documentations for the same
template.
The method acts as a straight setter now. The next patch will
rename it accordingly.
Bug: T285483
Change-Id: I3ffc202577e9a20fc7491234601ccd981113f866
Instead of faking entries in this.params, let's use a separate
tiny data structure to keep track of parameters we have seen so
far, and in which order.
This finally allows to easily distinguish between documented and
undocumented parameters.
Bug: T285483
Change-Id: Idf62b0661178a3bbef7e817edf016dbd572d415b
The so called "spec" class keeps track of parameters that have
been used before, no matter if documented via TemplateData or
not. Removed parameters are still "known" (i.e. have been seen
before).
This feature allows to easily find previously used parameters
names when an undocumented parameter was removed and the user
tries to add it again.
Bug: T285483
Change-Id: Ia1555eea87cd99e7a3f386f4279ec5a80fb98a79
In I04b8a14fbec7be5a1c4defabf92e94f694c1e638 we sepearted params from
aliases. There we missed that re-filling the parameters from the
template could re-add the aliases.
Bug: T285483
Bug: T285843
Change-Id: I1928b443a5f708bc8c57efa5ad0a86b5915b159c
While the term "canonical" is not wrong, I find it still
somewhat ambiguous.
1. "Canonical" could mean different things. E.g. is the order
of parameters as they appear in the article's wikitext the
"canonical" one? It's possible to argue like this, esp. if a
template doesn't have TemplateData documentation. In this case
the only order known is the one from the wikitext.
2. "Canonical" sounds like the parameters must be reordered.
But this should never happen. Not having dirty diffs is more
important than having the parameters in a specific order.
Bug: T285483
Change-Id: I23658d37fea50b727667677ac6a49066673b2135
These comments don't add any knowledge. The text is either
duplicated, or the method signatur says it already. Having
to read these comments just to realize that they don't give
any additional information is not helpful, even error-prone.
Change-Id: I014028b1e9311b831a22c37859b2130aed2e9539
Wait, what's going on here? This patch looks like it changes the
behavior of this code. But it doesn't. Here is what happened
before:
* Let's say a template contains 2 parameters, A and B.
* We don't know yet if these names are aliases.
* getParameterNames() returns [ "A", "B" ].
* extend() is called. The TemplateData documentation contains
the parameters "B" and "C". "C" does have an alias "A".
* extend() can't find "C" and adds it to the end, as if it's a
new parameter.
* extend() also iterates the aliases. For each alias it creates
a reference to the specification object. In this case a
reference from "A" to "C" is created.
* But "A" already exists. The position of "A" doesn't change,
but the specification now says it's an alias.
* getParameterNames() skips aliases. It skips "A" and instead
returns the new "C" from the end of the list.
This was the behavior before. It's unchanged, proven by the tests.
Change-Id: I04b8a14fbec7be5a1c4defabf92e94f694c1e638
The idea is to not actually store all these default values, but
fall back to the default only when needed.
Some more details:
* The only remaining property is ….name. The only reason to
have this property is to distinguish between aliases and
primary parameter names. This will be reworked in a later
patch.
* The description falls back to null because this is the
documented fallback, not undefined.
* The default value falls back to "", same as the auto-value.
Why not null you might ask. This is intentional. Both the
auto- and default value are effectively wikitext snippets,
while the example is a label in the VE UI.
Bug: T285483
Change-Id: I1be3cca18f9ad6fc1c16362b24633f7613f02539
This is done for two reasons:
1. It fixes the behavior of two methods in rare edge-case
situations. They aren't documented to return undefined.
2. It reduces the amount of stuff this class stores when it's
nothing but a default value anyway. Note this patch does this
for the template-level properties only. Another patch will do
the same for the parameter-level properties.
Bug: T285483
Change-Id: If2e4d56da1fa52e32dc94191f36d7dc6a1487829
This reflects much better how this method is meant to behave.
Note I will continue to remove documentation that doesn't
explain anything in addition to what the code already says.
Bug: T285483
Change-Id: I81fa8a5d9d0752f3aeac4015c9a27b50e054d4df
