Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

[ RFC ] Adding an optional allow list to hhast-lint.json #458

Open
lexidor opened this issue Apr 27, 2022 · 1 comment
Open

[ RFC ] Adding an optional allow list to hhast-lint.json #458

lexidor opened this issue Apr 27, 2022 · 1 comment

Comments

@lexidor
Copy link
Contributor

lexidor commented Apr 27, 2022

The following describes a suggestion. This issue is not meant to be implemented right away. This might not be a direction hhast wants to go in.


I would like to propose an allow_list for HHAST_FIXME comments, analogous to the hhconfig setting allowed_fixme_codes_strict introduced in hhvm 4.62. If a FIXME is found for a linter that is not in the allow list, an error like the following would be emitted. You may not use a comment to suppress a {{LintErrorName}} error. See lintFixmeAllowList in hhast-lint.json. The default value (unspecified) should be equivalent to "allow all fixmes" rather than "allow none".

Some linters should always be adhered to and adding a FIXME for them is always a bad idea. Other linters have valid uses for a FIXME. This use of await-in-a-loop is critical to the functionality.

function wait_until_service_is_ready_async(string $url, Deadline $deadline): Awaitable<bool> {
  while($deadline->hasTimeLeft()) {
    // HHAST_IGNORE_ERROR[DontAwaitInALoop]
    $status = await get_http_status_async($url);
    switch ($status) {
      case 200:
        return true;
      case 404:
        throw new RuntimeException(Str\format('Service %s does not exist', $url);
      case 0:
      case 503:
        $sleep = $deadline->microsLeftOr(25000) - 5000;
        if ($sleep <= 0) {
          return false;
        }
        //HHAST_IGNORE_ERROR[DontAwaitInALoop]
        await Asio\usleep($sleep);
      default:
        // etc...
    }
  }
  return false;
}

A configuration file for this project could look like:

{
  "roots": ["src/"],
  "lintFixmeAllowList": [
    "Facebook\\HHAST\\DontAwaitInALoopLinter",
  ],
  "overrides": [
    {
      "patterns": ["src/codegen/**"],
      "lintFixmeAllowList": [
        "YourCorp\\SomeLinter"
      ]
    }
  ]
}
@Atry
Copy link
Contributor

Atry commented Apr 27, 2022

Sounds reasonable to me, except for the property name lintFixmeAllowList. I would name it lintMarkerAllowList to cover other markers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants