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

Emit null instead of empty object for validateAshHooks #81

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

foolip
Copy link
Member

@foolip foolip commented Dec 16, 2019

This was left to minimize report.json changes in earlier
refactoring. Fixing it now should only affect the report.json
structure but not report.html.

@foolip foolip requested a review from dontcallmedom December 16, 2019 10:04
validate.js Outdated Show resolved Hide resolved
This was left to minimize report.json changes in earlier
refactoring. Fixing it now should only affect the report.json
structure but not report.html.
@foolip foolip changed the title Fix two TODOs about irregular report.json format Emit null instead of empty object for validateAshHooks Dec 16, 2019
@foolip
Copy link
Member Author

foolip commented Dec 16, 2019

I've trimmed this PR to only the remaining TODO.

@dontcallmedom
Copy link
Member

The diff shows

-      {
-        "repo": "w3c/dnt"
-      },
-      {
-        "repo": "w3c/qt3tests"
-      },
-      {
-        "repo": "w3c/web-annotation"
-      }
+      "w3c/dnt",
+      "w3c/qt3tests",
+      "w3c/web-annotation"
     ],

which I don't understand and wouldn't have expected. Any insight?

@foolip
Copy link
Member Author

foolip commented Dec 16, 2019

Oh, I didn't anticipate this, but it's because of this logic:

// Push just the repo name string if there are no details, and otherwise
// add it as a `repo` property to the details object.
if (details === null) {
allerrors[type].push(fullName(repo));
} else {
allerrors[type].push({repo: fullName(repo), ...details});
}

@dontcallmedom maybe this should be changed to always use the object form? If such a change is landed first, this change should then be a no-op.

@dontcallmedom
Copy link
Member

changing to always use object form makes sense to me, but will probably need changes in downstream usage of the JSON results (which points toward the need of setting up a JSON schema for the results I think)

@foolip
Copy link
Member Author

foolip commented Dec 16, 2019

As it happens I was just working on using a JSON schema to validate w3c.json, if that works out it should be straightforward (ish) to use it for report.json as well.

Let's leave this PR hanging, it's not urgent in any way.

Base automatically changed from master to main February 5, 2021 08:22
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