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

Web IDL study: detect event handlers with no matching event #785

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Sep 11, 2024

As discussed in w3c/webref#1216, when an interface defines an event handler, there should always be an event named after the event handler that targets the interface. The new noEvent anomaly detects situations where the event is missing.

The analysis reports the two anomalies mentioned in w3c/webref#1216 (comment)

(The analysis reports additional anomalies when run on the raw extracts. That's normal and due to missing event targets that get added during curation)

I do not know yet to what extent we want to make that a guarantee for the Webref events package. First exception-to-the rule is easy to explain. The second one for RTCIceTransport.onerror will hopefully be fixed at some point, the overall question being: what patching do we do when we bump into a problem that does not get fixed immediately?

When an interface defines an event handler, there should always be an event
named after the event handler that targets the interface. The new `noEvent`
anomaly detects situations where the event is missing.

The analysis reports the two anomalies mentioned in:
w3c/webref#1216 (comment)

(The analysis reports additional anomalies when run on the raw extracts. That's
normal and due to missing event targets that get added during curation)
Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

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

looks great, a couple of nits

test/study-webidl.js Outdated Show resolved Hide resolved
src/lib/study.js Outdated
@@ -149,6 +149,11 @@ const anomalyGroups = [
guidance: 'See the [`[Exposed]`](https://webidl.spec.whatwg.org/#Exposed) extended attribute section in Web IDL for requirements.'
},
{ name: 'invalid', title: 'Invalid Web IDL' },
{
name: 'noEvent',
title: 'Suspicious event handlers',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: 'Suspicious event handlers',
title: 'Event handlers needing review',

not great either, but "suspicious" feels a bit strong

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree "suspicious" isn't good. Looking at the title of this PR, I'm thinking "Event handlers with no matching events" could be a good enough title in practice.

@dontcallmedom
Copy link
Member

The guarantee we could consider for webref would be that each event can be tied to the relevant eventhandler attribute(s) (or be annotated as not having such a thing). Then we could use the naming best practice to fill it by default, and if/when that best practice should fail, we could presumably have an override through the event patching mechanism.

With that said, I don't think we have such a demand from any consumer at this point, so I don't think we need to bother with this at this point.

@tidoust
Copy link
Member Author

tidoust commented Sep 11, 2024

With that said, I don't think we have such a demand from any consumer at this point, so I don't think we need to bother with this at this point.

In the meantime, it could be useful to enable detection of these anomalies within the job, so that we get alerted when a new case arises. One caveat though. The job runs analyses on the raw crawl results and this analysis would better run on the curated results, it's useless (and noisy) to report anomalies that are triggered by another anomaly that we'd have already detected and reported. Amending the job to run a second analysis on the curated branch should be relatively easy.

@tidoust tidoust merged commit fb6cec0 into main Sep 11, 2024
1 check passed
@tidoust tidoust deleted the noevent branch September 11, 2024 10:30
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