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

test framework + spec #10

Merged
merged 13 commits into from
Mar 9, 2023
Merged

test framework + spec #10

merged 13 commits into from
Mar 9, 2023

Conversation

marcoscaceres
Copy link
Contributor

@marcoscaceres marcoscaceres commented Jul 5, 2021

@dontcallmedom, @tidoust, as we discussed last week. Here is the initial version + testing framework.

For testing, I'm just using Karma + Jasmine.

I'm generating the BS specs via curl and doing a local respec build.

My idea is just to add various definitions to src files, and make sure ReSpec and Bikeshed do the same thing (already found a bug :)) .

@dontcallmedom
Copy link
Member

could the generated files be built on demand rather than committed to the repo?

README.md Outdated
A a dfn-definition MUST have the following attributes:

- `id` attribute, unique to the document.
- `data-dfn-type`, with one of type values.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should recognize it as optional for <dfn> elements (that's how reffy handles it, and I suspect shepherd too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going by 4 of the contract:

data-dfn-type MUST be provided, and set one of the accepted values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference tho. ReSpec treats them as optional, but I sent a PR to ReSpec to always include them: https://github.com/w3c/respec/pull/3667.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a reasonable default especially if we consider that people could possibly create spec markup manually (no one really does that, but the intro suggests that is a possibility): <dfn data-dfn-type="dfn"> looks very redundant when compared to <dfn>.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on it being super-redundant. And there are manually maintained specs, e.g. WebGL 1 & 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd does some inference if it's missing, for legacy specs. I don't think we want to specify those inference rules; they're heuristic and kinda complex and specific to the styles that happen to be used by the legacy specs in question. (They're based on examining the text, the ID, and I think there's some matching to WebIDL in the document as well.)

So I def prefer continuing to require it.


### Exporting definitions (`data-export`)

Exported definitions MUST include a `data-export` attribute. The attribute's value can be missing or the empty string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should recognize the intersection with dfn-type that makes some definitions exported without a data-export attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... that seems to be a Bikeshed-ism that relies on tree structure (ReSpec doesn't support dfn-type on container elements).

I think the tooling should be adding data-export in those situations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @marcoscaceres and think that's already the case in practice, dfn-type being just a tooling artefact to produce data-dfn-type attributes in the resulting spec. Typically, Reffy does not know anything about dfn-type and only looks for data-dfn-type attributes. I don't think that the contract document should mention dfn-type at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me clarify what I meant - I wasn't arguing about dfn-type vs data-dfn-type (I was just foolishly saving typing 5 characters). I was saying some value of data-dfn-type are exported by default, some are not exported by default, and this should be reflected in the definition of exported definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for clarifying @dontcallmedom.

So, let's reframe this because I think this is really important. Tooling can add data-dfn-type, but (!!!) they must add data-export="" for a definition to actually be classified as exported.

Put differently, if a definition is "exported" must not be implied by the presence of data-dfn-type. Only the explicit presence of data-export="" makes something exported.

Agree?

I know that adds redundancy, but it also gets rid of any ambiguity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I understand what you're suggesting now: the fact that some data-dfn-type are automatically exported is not a rule of the type, but a default that authoring tools apply when generating the output.

This has the merits of disentangling the two axes; I'm a bit worried of the impact on making this work for non-bikeshed / non-respec maintained documents, but we can revisit this when this becomes a more realistic question to consider :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, putting the dfn-* attributes on ancestors is a Bikeshed-ism; it gets turned into data-dfn-* attributes directly on the <dfn> by generation time.

A mention of this is very likely reasonable, in case people run into this document when looking for how to format their dfns, but it's definitely not part of the contract itself.

Put differently, if a definition is "exported" must not be implied by the presence of data-dfn-type. Only the explicit presence of data-export="" makes something exported.

I dunno if I agree with this. I think in the absence of an explicit export/noexport, it's reasonable to apply the same defaulting that Bikeshed does. (But we should still require tooling to add it explicitly.)

README.md Outdated
No other HTML elements are recognized as defining a term.

#### dfn
A a dfn-definition MUST have the following attributes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if rather than MUST (which ideally should apply to agent, not to models) if we should declare what a conforming definition is (a dfn element with an id attribute, or or h* element with an id and a dfn-type attribute).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree. I'll change them to statements of fact.

README.md Outdated

A `data-noexport` attribute means a definition is intended for private use by a specification.

Other specifications MUST NOT reference or link to definitions marked as `data-noexport`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specs aren't part of the conformance landscape :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS has put requirements on spec authors before. ^_^ Since this doc is explicitly about writing specs, I think having conformance requirements is just fine, and in fact exactly correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(That said, this should be downgraded to a SHOULD NOT. It can be appropriate to refer to an unexported term sometimes, but it needs a good reason.)

### Types of definitions (`data-dfn-type`)

Every exported definition MUST have a `data-dfn-type`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to insert a summary table that for each type indiciates:

  • whether it's exported by default or not
  • whether it's case sensitive or not
  • whether data-dfn-for is required for the said type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree with this. But for each type, there are specific requirements that I need to work out... like I don't know what data-dfn-for for "argument" is right now... I guess it would be "interface/method", but need to check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct. The for is just whatever construct is namespacing the value; arguments clearly are unique only up to their method (in fact, to within one overload of a method).

@marcoscaceres
Copy link
Contributor Author

Alright... this is probably enough for us to start to incrementally build on.

@marcoscaceres marcoscaceres marked this pull request as ready for review August 4, 2021 06:22
@jyasskin
Copy link

jyasskin commented Feb 1, 2023

How would you feel about merging this, and then iterating on the details?

@tidoust
Copy link
Contributor

tidoust commented Feb 1, 2023

That would be good. Do we still have a maintainer for this, though? @marcoscaceres, are you still able to lead the effort? If not, @jyasskin, would you be able to take that on?

@jyasskin
Copy link

jyasskin commented Feb 2, 2023

I think @tabatkins and I can take care of this, and probably @sidvishnoi can keep track of the Respec side even if @marcoscaceres turns out not to have time. I'm not 100% confident of maintaining a unified test suite, but the documentation work is just moving the work Tab was doing in the Bikeshed docs anyway.

@tidoust
Copy link
Contributor

tidoust commented Mar 9, 2023

Not sure if you're waiting on further feedback, @jyasskin. That sounds like a good plan to me, and the lack of responses suggests that no one else is willing to step up ;)

@jyasskin
Copy link

jyasskin commented Mar 9, 2023

I'd lost track of the fact that I can merge it now. Doing so.

@jyasskin jyasskin merged commit 1cedbe1 into main Mar 9, 2023
@jyasskin jyasskin deleted the base_stuff branch March 9, 2023 17:10
@marcoscaceres
Copy link
Contributor Author

Sorry for missing this... yeah, I won't have time :(

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.

5 participants