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

Minor editorial cleanup / refactor of Manifest processing steps #1066

Merged
merged 3 commits into from
Jan 25, 2023

Conversation

mgiuca
Copy link
Collaborator

@mgiuca mgiuca commented Jan 20, 2023

This change (choose at least one, delete ones that don't apply):

  • Makes editorial changes (changes informative sections, or changes normative sections without changing behavior)

Commit message:

Minor editorial cleanup / refactor of Manifest processing steps.

  • Fixed call to "process the id member" (removed the document URL argument which is not actually accepted by that algorithm).
  • Removed unnecessarily complex for loops over ~2 members when it's more readable to just have a separate step for each member.
  • Use Respec-style syntax instead of HTML.
  • Moved default values into individual processing steps. This keeps the relevant info about the default values of members closely related to the other relevant material about that member. It's also consistent with how the rest of the members (e.g. scope) treat default values. And prevents possible errors where the incorrect default value is used by an intermediate step in between assigning the default and assigning the actual value.

Pre-work for #668.

Person merging, please make sure that commits are squashed with one of the following as a commit message prefix:

  • editorial:

Preview | Diff

@mgiuca mgiuca requested a review from marcoscaceres January 20, 2023 00:20
Directly process name, short_name, theme_color and background_color.

(Note: short_name was redundantly processed in the following step. We
could have just removed that step, but I think it is more readable to
just remove the for loop.)
@mgiuca mgiuca force-pushed the processing-editorial branch from 282d2aa to aa2a052 Compare January 23, 2023 00:57
@mgiuca
Copy link
Collaborator Author

mgiuca commented Jan 23, 2023

I'm getting a host error connecting to labs.w3.org which is blocking the merge (I think it was ready to merge before but I rebased over #1067).

@mgiuca
Copy link
Collaborator Author

mgiuca commented Jan 23, 2023

I'm also getting a Respec error:

Document is not associated with a W3C group. Defaulting to 'base' status.

How to fix: Use the group configuration option to associated this document with a W3C group.

But it isn't caused by this PR.

@mgiuca mgiuca force-pushed the processing-editorial branch from aa2a052 to a1b2b54 Compare January 23, 2023 01:53
@mgiuca
Copy link
Collaborator Author

mgiuca commented Jan 23, 2023

@marcoscaceres Something is broken at the manifest factory. Every build is failing with what looks like network errors since Friday.

https://github.com/w3c/manifest/actions/workflows/auto-publish.yml

The failures all start with "[ERROR] Network error loading highlighter".

Then as of this morning a new failure was added: "ROR] Plugin w3c/group took too long.".

This keeps the relevant info about the default values of members closely
related to the other relevant material about that member. It's also
consistent with how the rest of the members (e.g. scope) treat default
values. And prevents possible errors where the incorrect default value
is used by an intermediate step in between assigning the default and
assigning the actual value.
@mgiuca mgiuca force-pushed the processing-editorial branch from a1b2b54 to d720167 Compare January 25, 2023 00:56
@mgiuca mgiuca merged commit 80310d0 into w3c:main Jan 25, 2023
@mgiuca mgiuca deleted the processing-editorial branch January 25, 2023 02:22
github-actions bot added a commit that referenced this pull request Jan 25, 2023
SHA: 80310d0
Reason: push, by mgiuca

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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