Major rewrite to make things more consistent and extensible #664

tidoust · 2024-08-02T16:26:10Z

The code in Strudy did the same thing in multiple ways:

The main CLI was linked to the first type of analysis reports, that still get published to w3c/webref-analysis.
A few CLIs were there, used mainly for exploration purpose.
The study modules could sometimes be used as CLI, for no apparent reason.
The study modules used different conventions for inputs/outputs
The reporting code was somewhat separated from the rest, whereas it could be useful to extend it.
Preparing flexible reports and amending the "file an issue" workflow was a bit tedious.

This rewrite attempts to solve the above by:

Rewriting study modules to expose one similar study function
Adding a generic study module that knows about all anomaly types, dispatches the study to the different study modules depending on the request, and can structure and format the result as needed in markdown to file one or more issues.
Adding a few tests to be more confident that the analyses are doing the right thing!
Updating the entry points to use the new study module
Getting rid of all side CLIs

Breaking changes: Yes! ;)
Issues reporting should work as before, but any external project making use of Strudy will need to adjust calls and/or use of the CLI. Main breaking changes are:

Previous CLIs are gone. Only the main strudy CLI remains. And file-for-review, but that's more intended for internal usage.
The CLI takes different parameters and options and produces similar but different results.
Dependencies reports can no longer be generated. But we can add that back in the future with additional CLI commands on top of the inspect one.
Diff reports can no longer be generated.
HTML reports can no longer be generated.
Once released, the NPM package will only export a generic study function.

Updates to w3c/webref-analysis will no longer be possible as a result. I'm not aware of anyone relying on that project, so it seems fine to shelve it and archive the repository.

Additional notes:

We'll probably want more flexibility on how to structure the reports over time, e.g. for Consider grouping broken link reports in a single issue by repo (for multi-spec repos) #593.
I stuck to brokenLinks for now in the job but we'll probably want to start reporting issues on additional anomaly types. That's the overall motivation for these changes!
Tests on the CLI remain fairly minimal for now.

**IMPORTANT: Work in progress, not yet functional!** The code in Strudy did the same thing in multiple ways: - The main CLI was linked to the first type of analysis reports, that still get published to `w3c/webref-analysis`. - A few CLIs were there, used mainly for exploration purpose. - The study modules could sometimes be used as CLI, for no apparent reason. - The study modules used different conventions for inputs/outputs - The reporting code was somewhat separated from the rest, whereas it could be useful to extend it. - Preparing flexible reports and amending the "file an issue" workflow was a bit tedious. This rewrite attempts to solve the above by: - Rewriting study modules to expose one similar study function - Adding a generic study module that knows about all anomaly types, dispatches the study to the different study modules depending on the request, and can structure and format the result as needed in markdown to file one or more issues. - Adding a few tests to be more confident that the analyses are doing the right thing! - **(Not done yet)** Updating the entry points to use the new study module - **(Not done yet)** Getting rid of all side CLIs

dontcallmedom

a first pass at review the refactoring, with a few minor nits; as always, a superb set of improvements :)

src/lib/study-dfns.js

src/lib/study.js

Time to rewrite the CLI. The CLI now gets divided into commands. The only available command for now is `inspect`, but a `view` command could perhaps be created afterwards to create views on the crawl report, such as the previous dependencies report (what specs have a normative reference on a given spec). New options got added to the CLI: ``` -f, --format <format> report markdown or json (default: "markdown") -i, --issues <folder> report issues as markdown files in the given folder -m, --max <max> maximum number of issue files to create/update (default: 0) --structure <structure> report structure (default: "type+spec") --update-mode <mode> what issue files to update (default: "new") -w, --what <what...> what to analyze (default: ["all"]) ``` Library code updated to fix a few bugs, integrate feedback, and complete the logic where needed. Tests remain fairly minimal for now.

The `--update-mode` option now also accepts an `old` value that preserves existing issue files by default, except when the analysis no longer detects any error for them.

The script used to run the study *and* handle Git and GitHub commands. It now expects that some analysis got run with the main strudy CLI, and only handles Git and GitHub commands. One update is that the script can also create pull requests for issue files that got deleted during the analysis (because study did not reveal any issue with the underlying spec anymore).

Delete all the things! The previous CLIs should no longer be needed as the main Strudy CLI now should take care of everything. The `study-crawl` CLI (and companion `generate-report`) is a sort of exception to the rule: all anomalies it reported are now covered by the CLI, but the CLI could also output a dependencies report, compute a diff between two reports, and generate an HTML report. New Strudy CLI no longer can do any of that. This update drops the files anyway. We could perhaps keep them around, but we haven't maintained the code, and `w3c/webref-analysis` is, I think, the only project that uses it and I propose to shelve that project now that we have, at least theoretically, a more flexible way to create issue reports. The NPM package would only export the main `study` function, because there shouldn't be any need to export the individual study functions such as `studyWebIdl` anymore. We'll have to update Webref tests accordingly. Project dependencies adjusted to drop `node-pandoc`... and `node-fetch` which was no longer being used in any case.

This refreshes the boilerplate text in existing issue files so that the CLI can more easily detect whether a change is warranted, and stops reporting that the files need to be updated in particular.

Forgotten in a previous commit

tidoust · 2024-08-22T14:18:05Z

PR should now be ready for review. I updated the initial description. I also tried to split further updates into coherent commits to hopefully ease review. I guess the best way to see what this does is to try it locally. For example:

node strudy.js inspect --help
node strudy.js inspect ../webref --issues issues --what brokenLinks discontinuedReferences

node src/reporting/file-issue-for-review.js --help
node src/reporting/file-issue-for-review.js --dry-run --max 5

This will make changes to your local issues folder, which can then be rolled back with:

git restore issues/
git clean -f issues/

src/reporting/file-issue-for-review.js

dontcallmedom · 2024-08-22T15:15:46Z

I had reviewed the commits as they come in and they LGTM.

I also tried the commands locally - the last one fails due to the comment I raised (although I'm not sure if it points to another failure than just the code optimism)

src/reporting/file-issue-for-review.js

tidoust force-pushed the rewrite branch from 785dfc5 to 7a89322 Compare August 2, 2024 22:46

dontcallmedom reviewed Aug 19, 2024

View reviewed changes

tidoust mentioned this pull request Aug 21, 2024

dfns analysis: is the workaround done for Respec still needed? #673

Open

tidoust added 6 commits August 21, 2024 14:31

Handle old issue files that can now be deleted

2b4982d

The `--update-mode` option now also accepts an `old` value that preserves existing issue files by default, except when the analysis no longer detects any error for them.

Adjust boilerplate text in existing issue files

4ee5068

This refreshes the boilerplate text in existing issue files so that the CLI can more easily detect whether a change is warranted, and stops reporting that the files need to be updated in particular.

Stop exporting studyWebIdl

5f9271b

Forgotten in a previous commit

tidoust marked this pull request as ready for review August 22, 2024 14:18

tidoust requested a review from dontcallmedom August 22, 2024 14:18

dontcallmedom reviewed Aug 22, 2024

View reviewed changes

src/reporting/file-issue-for-review.js Show resolved Hide resolved

Restrict lookup to ".md" files in issues folder

adc0eeb

dontcallmedom reviewed Aug 22, 2024

View reviewed changes

src/reporting/file-issue-for-review.js Outdated Show resolved Hide resolved

src/reporting/file-issue-for-review.js Outdated Show resolved Hide resolved

Filter out empty strings (and add reminder)

18f08e6

dontcallmedom approved these changes Aug 23, 2024

View reviewed changes

dontcallmedom merged commit 7bd07aa into main Aug 23, 2024
1 check passed

dontcallmedom deleted the rewrite branch August 23, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major rewrite to make things more consistent and extensible #664

Major rewrite to make things more consistent and extensible #664

tidoust commented Aug 2, 2024 •

edited

Loading

dontcallmedom left a comment

tidoust commented Aug 22, 2024

dontcallmedom commented Aug 22, 2024 •

edited

Loading

Major rewrite to make things more consistent and extensible #664

Major rewrite to make things more consistent and extensible #664

Conversation

tidoust commented Aug 2, 2024 • edited Loading

dontcallmedom left a comment

Choose a reason for hiding this comment

tidoust commented Aug 22, 2024

dontcallmedom commented Aug 22, 2024 • edited Loading

tidoust commented Aug 2, 2024 •

edited

Loading

dontcallmedom commented Aug 22, 2024 •

edited

Loading