Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Emit null instead of empty object forvalidateAshHooks#81

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
foolip wants to merge1 commit intomain
base:main
Choose a base branch
Loading
fromreport-json-todos

Conversation

@foolip
Copy link
Member

@foolipfoolip commentedDec 16, 2019•
edited
Loading

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.

This was left to minimize report.json changes in earlierrefactoring. Fixing it now should only affect the report.jsonstructure but not report.html.
@foolipfoolip changed the titleFix two TODOs about irregular report.json formatEmit null instead of empty object forvalidateAshHooksDec 16, 2019
@foolip
Copy link
MemberAuthor

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
MemberAuthor

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
MemberAuthor

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 frommaster tomainFebruary 5, 2021 08:22
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dontcallmedomdontcallmedomdontcallmedom left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@foolip@dontcallmedom
[8]ページ先頭
©2009-2025 Movatter.jp