endojs / Jessie

Tiny subset of JavaScript for ocap-safe universal mobile code
Apache License 2.0
279 stars 16 forks source link

max-len suppression causes false positive unused #115

Closed turadg closed 5 months ago

turadg commented 5 months ago

In agoric-sdk we started reporting unused eslint directives. This mysterious one started showing up,

/opt/agoric/agoric-sdk/packages/internal/src/action-types.js
  1:1  warning  Unused eslint-disable directive (no problems were reported from 'max-len')

/opt/agoric/agoric-sdk/packages/internal/src/batched-deliver.js
  1:1  warning  Unused eslint-disable directive (no problems were reported from 'max-len')

even though max-len isn't mentioned in the repo.

It turns out it's just in files with // @jessie-check and it's due to this content prefixing: https://github.com/endojs/Jessie/blob/96cabd9b688f28c0ea7576124843b97998316e36/packages/eslint-plugin/lib/processors/use-jessie.js#L43

@michaelfig is that necessary?

michaelfig commented 5 months ago

is that necessary?

It was the only solution I could find to the problem where eslint's default max-len rule would get tripped up on the length of the Jessie preamble. Is there a way to tell the unused directive detector to ignore this rule?

turadg commented 5 months ago

Is there a way to tell the unused directive detector to ignore this rule?

I don't think so. Pretty rare case to have to support.

Could the preamble have some newlines? Alternately, we could just assume nobody has max-len enabled since it has been deprecated since v8.53.0: https://eslint.org/docs/latest/rules/max-len

michaelfig commented 5 months ago

Could the preamble have some newlines?

If there are any newlines, then eslint reports incorrect line numbers when an error or warning is detected.

Alternately, we could just assume nobody has max-len enabled since it has been deprecated since v8.53.0

Yes, that would be preferable. It's an easy fix, but requires prioritisation.

turadg commented 5 months ago

I tried to release with these changes but couldn't figure out the steps from https://github.com/endojs/Jessie/pull/109