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

Add arraybuffer-base64 and digital identities specs #1204

Merged
merged 13 commits into from
Feb 19, 2024
Merged

Conversation

dontcallmedom
Copy link
Member

No description provided.

@dontcallmedom dontcallmedom mentioned this pull request Feb 19, 2024
3 tasks
Copy link
Member

@tidoust tidoust left a 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"]
}

@dontcallmedom
Copy link
Member Author

I see your point wrt null; I had started with adding an exception to the code, but this one felt special enough that it didn't feel quite right there either.

I'm not sure I like the delete or skip alternative all that much either, so I'll revert back to hardcoding the exception in compute-shortname for now.

@@ -179,6 +179,7 @@ function completeWithSeriesAndLevel(shortname, url, forkOf) {
// digits, as in "iso18181-2".
if (seriesBasename.match(/^ecma-/) ||
seriesBasename.startsWith("iso") ||
seriesBasename.endsWith("base64") ||
Copy link
Member

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:

Suggested change
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/.

Copy link
Member Author

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?

Copy link
Member

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.

@tidoust tidoust merged commit a008958 into main Feb 19, 2024
1 check failed
@tidoust tidoust deleted the newspecs-1203 branch February 19, 2024 14:45
tidoust added a commit that referenced this pull request Sep 10, 2024
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.
tidoust added a commit that referenced this pull request Sep 10, 2024
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.
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

Successfully merging this pull request may close these issues.

2 participants