Thoughts on the recent PRs re: eslint, prettier?

[tony]

In JS projects it's pretty common to find a setup with both of those, the reason why is editors and CI work with the prettier/eslint very well.

Any thoughts on adopting these here and in the other projects for consistency? If there is I can make PRs across the others (and if there's a prettier/eslint config)

I think the value equation is there. eslint catches a lot of basic mistakes and prettier keeps code consistent resulting in easier to read diffs / reviews in the long run.

Endgame: My idea is to have a basic CI setup with eslint / prettier and perhaps checking for the exit code of npx ncc build src/index.js to assure it builds without failing. T

[sshniro]

Hi @tony , yes this is a good suggestion, PR welcome!

[tony]

@sshniro Good! If you have opinions on the eslint (#26, #27) /prettier (#28) config let me know

[tony]

@sshniro Where do the actions sit as a priority on the roadmap of @zaproxy maintainers? Just trying to gauge the cadence on reviews and releases (so I don't create too many PRs!)

P.S. also thank you for the project and your time!

[kingthorin]

Hi @tony we do appreciate the PRs and work behind them. Unfortunately between the ramp up to holidays, log4shell, volunteers/priorities the team just hasn’t been able to get to these yet. For the time being I’d suggest holding off on further PRs, but don’t give up. These will be dealt with and then it’ll make sense to continue.

Thanks again for your interest and effort!

[psiinon]

Seconded! Thanks @tony and sorry we havnt got to your PRs yet but they are appreciated and we will get to them asap...

[tony]

@kingthorin @psiinon Thank you for your very courteous responses. Noted on recent security log4j issue as well.

I will hold off on PRs in the mean time. Best, Tony

[thc202]

For consistency it might be better to use the same linter as the HUD repo.

[tony]

The only dependency I see on HUD is a eslint-plugin-no-sanitized. Anything else?

I also don't see a configuration in there (yet)

I assume if something were to be shared, it'd be a configuration file (e.g. .eslintrc). If repeated heavily in an org across repos, a shareable config is sometimes published to NPM.

[thc202]

The HUD project uses xo.

[tony]

xo looks good. Perhaps I can update #25/#26/#27/#28 to reflect that?

[tony]

1. I think a separate issue should be made to do xo or decide on a linter/formatting tool. I think its one of those things where there needs to be a decision convened before starting on a PR 😆 Please feel free to reopen or let me know if you want to reopen

2. I bulk closed a lot of PRs yesterday, I look back and that sounded abrupt - I looked at the PR list last night and I wanted to trim down the backlog to build consensus before making PRs

   Normally in situations where I have PRs open I'd rebase and ping

   What I've done lately is been maintaining independent PRs split out from a fork and its difficult to switch back to them individually, test, and stay on top of them. In this case, there's development loop since it's a github action (I went into the switching cost a bit on the list in December last year) and switching cost between PRs and a main fork.

[psiinon]

Hi @tony - we really do appreciate the help and suggestions but some of the PRs could definitely have done with a bit of discussion first ;)
How about raising issues for all of the things you think need addressing? I think we agree with pretty much all of them its just a case of making sure they are done the right way.
And us finding time to give suitable feedback of course :/
A set of focussed issues would help us focus on provinding more timely feedback and should make any subsequent PRs much easier to merge.
Many thanks!

[tony]

@psiinon

• Move optional params (or all params) to an object literal

  https://github.com/zaproxy/actions-common/pull/22/files moves artifactName and allowIssueWriting to an object

  I think there's a good case for making all params be an object literal.

  1. The reason why is the order doesn't matter and they always have to be explicitly defined. This helps avoids mistakes since there isn't any type safety or inferences that can be gleaned from positional args in ecmascript

  2. More convenient for actions using the library itself if future parameters are introduced.

  3. Easy to pass in via destruturing

• Multiple issues being created: Scan report: Duplicate issue creation when artifactName passed #20

  I am not sure about this yet but I am currently using this successfully: https://github.com/zaproxy/actions-common/pull/21/files

• Formatting / Linter configs
  if .prettierrc and .eslintrc are in the repo, editors pick it up and it handles formatting + CI can check it

  per @thc202 here the HUD project uses xo. That's fine with me.

  One area of concern is this project has its own plugins for editors and build systems (reinventing the wheel), and xo itself or any of the plugins could potentially fall out of maintenance/have bugs and no easy escape hatch other than dropping xo and using the eslint/prettier/etc directly. since it conglomerates multiple tools into one trying to trace rules that conflict with each other can be time consuming

  it could be more portable in the end to use the tools directly. eslint, prettier or eslint-plugin-prettier and decide on a config.

Thoughts on any of these? Worth reopening or creating a new one(s) for any of them?