Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging custom keys from computed meta + in-document metadata doesn't seem to work #1161

Closed
domenic opened this issue Jan 4, 2018 · 6 comments

Comments

@domenic
Copy link
Collaborator

domenic commented Jan 4, 2018

This can be seen in whatwg/dom#553 and whatwg/mimesniff#54 with #1155 applied. I haven't tried for a minimal repro, but the basic setup is:

computed-metadata.include:

{
  "!Participate": [
    "<a href=https://github.com/whatwg/[SHORTNAME]>GitHub whatwg/[SHORTNAME]</a> (<a href=https://github.com/whatwg/[SHORTNAME]/issues/new>new issue</a>, <a href=https://github.com/whatwg/[SHORTNAME]/issues>open issues</a>)",
    "<a href=https://wiki.whatwg.org/wiki/IRC>IRC: #whatwg on Freenode</a>"
  ],
  ...
}

spec.bs:

<pre class=metadata>
...
!Participate: <a href="https://www.w3.org/Bugs/Public/buglist.cgi?product=WHATWG&amp;component=MIME&amp;resolution=---">legacy open bugs</a>
</pre>

The fix here should be sure to group them together according to the Metadata Order.

@tabatkins
Copy link
Collaborator

Spotted the problem - the default "join" strategy between different sources for custom metadata (anything with a !) is to override whatever was specified previously. That seems like it's probably a bad behavior - it's inconsistent with the default strategy within a source, which just appends them to the list and displays them all.

I'll change it to be consistent and append in both cases. Let me know if I need to offer more functionality here and give you a choice between the two (like a "clear" option or something).

@tabatkins
Copy link
Collaborator

Done, and confirmed that it works as desired with your reduction. They'll append in priority order - defaults, then in-document, then command line, then computed-metadata, and maintain their relative ordering within each of those categories.

@domenic
Copy link
Collaborator Author

domenic commented Jan 4, 2018

Hmm, for our purposes computed-metadata is higher priority than in-document, but it's not a big deal. (It only applies to two specs with this legacy bugs situation; no other metadata is affected.)

@tabatkins
Copy link
Collaborator

Yes, that's what I mean - the list I gave is the order they'll show up in. So computed-metadata will always show up last.

@domenic
Copy link
Collaborator Author

domenic commented Jan 4, 2018

Yeah, whereas it'd be kind of nicer if those "defaults" showed up first, whereas the in-document "extras" showed up at the bottom of the "Participate" list. But again, not a big deal.

@tabatkins
Copy link
Collaborator

Doing that reasonably would require me to finally do the correctness-rewrite of metadata handling I've been intending for a while. Something for the future. ^_^

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

No branches or pull requests

2 participants