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

WIP: Support multiple spec builds. #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jeremyroman
Copy link

Aimed at #18.

This is rather work-in-progress (missing unit tests, and I haven't been able to figure out how you integration-test this -- so it might actually be broken) but I was hoping for early feedback before I sink more time into this, in case you fundamentally disagree with the direction.

Essentially, this extends the config format to allow this construction:

{
  "specs": [
    {"src_file": "foo.bs", "type": "bikeshed"},
    {"src_file": "bar.bs", "type": "bikeshed"}
  ]
}

A desugaring step propagates any options specified outside "specs" to each element that doesn't provide the same option, allowing common params to be specified only once. Finally, exactly one of "specs" and "src_file" must be specified at the top level. If "src_file" is specified at the top level, it is effectively rewritten to "specs": {"src_file": "..."} which has the same behavior.

A PR is treated as multi-file if either it has multiple specs or any spec it does have is multi-file. This is intended to preserve existing behavior and URLs (since all existing .pr-preview.json have a single spec). Due to the complexity of the existing multi-file processor (Wattsi) and dealing with the potential for output file collisions, at present configs that would have a Wattsi config side-by-side with another spec are outright rejected, though this could be loosened with more work if necessary.

@jeremyroman
Copy link
Author

@tobie Would you mind taking a look and seeing if this is headed in the right direction and giving me pointers to how I could make it ready for landing (I recognize it presently isn't quite there).

@tobie
Copy link
Owner

tobie commented Oct 7, 2021

Hey, thanks for getting started on this.

I have very little time to commit to reviewing this atm. Skimming through the PR, the approach feels reasonable, but I haven't dug in, much, tbh.

It would be useful to reduce the noise a little by avoiding some of the refactoring and cleanup going on, or maybe splitting those in a separate commit. Explaining why you're creating a separate SpecBuilder and what its role is would also be useful.

There are no integration tests for the app at all unfortunately, as was originally just a lightweight script to help me figure out HTML diffs for the WebIDL spec.

That's one of the reasons I had been seeking funds. I'm keen to get your work out, but I'm also very concerned about the risks of deployment something that's hard to test before doing so.

The review will need to be extremely thorough and whatever you can do to help with this (by touching as little as possible and splitting things up) will definitely help.

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