-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add arraybuffer-base64 and digital identities specs #1204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of using the presence of null
as a signal. First thing I want to do when I see a property with a null value is to remove it as useless.
We already handle a few exceptions in src/compute-shortname.js
, including for published ECMAScript specs. I would rather extend that list to TC39 proposals.
Alternatively, a more generic mechanism could be to add an explicit delete
(or skip
) property in specs.json
, something like:
{
"url": "https://tc39.es/proposal-arraybuffer-base64/spec/",
"delete": ["seriesVersion"]
}
I see your point wrt I'm not sure I like the |
src/compute-shortname.js
Outdated
@@ -179,6 +179,7 @@ function completeWithSeriesAndLevel(shortname, url, forkOf) { | |||
// digits, as in "iso18181-2". | |||
if (seriesBasename.match(/^ecma-/) || | |||
seriesBasename.startsWith("iso") || | |||
seriesBasename.endsWith("base64") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was thinking of a more generic "no series version for TC39 proposals" exception, since in practice these proposals do not, and should not, contain any version number.
Looking into it, the exception could actually be "no series version for TC39 specs" since, at least for now, we don't try to keep track of versions of TC39 specs:
seriesBasename.endsWith("base64") || | |
url.match(/^https:\/\/tc39\.es\//) || |
This also allows to drop the first seriesBasename.match(/^ecma-/)
exception as underlying URLs also start with https://tc39.es/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much more elegant, indeed :) will fix
Maybe the generalization of this is using an allowlist for organizations that do use series version? (or a blocklist of the reverse); that's probably not practical at compute-shortname stage, but could be done in a clean up phase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah well, the approach based on the URL is not going to work as-is, due to the fact that shortname was made explicit, which effectively replaces the URL in the call to computeShortname
. See #1219.
I'll update to a check on the shortname instead.
That shortname isn't automatically extracted atm
The digital-credentials repo was added to the monitor list back in January (#1174). The digital-identities spec was added shortly afterwards (#1204), but was using a different repository at the time. Then digital-identities became digital-credentials, but we missed that. Instead we recently reviewed the list of monitored specs and decided to add digital-credentials (#1477). That means we have twice the same spec in the list. This update drops the digital-identities entry and adds the name as a former name of digital-credentials, since that's the name under which the spec is now progressing.
The digital-credentials repo was added to the monitor list back in January (#1174). The digital-identities spec was added shortly afterwards (#1204), but was using a different repository at the time. Then digital-identities became digital-credentials, but we missed that. Instead we recently reviewed the list of monitored specs and decided to add digital-credentials (#1477). That means we have twice the same spec in the list. This update drops the digital-identities entry and adds the name as a former name of digital-credentials, since that's the name under which the spec is now progressing.
No description provided.