Some trivial cleanups came up during another PR's review. #789
Conversation
Also wrapped some very long lines.
qingxinwu
changed the title
| @@ -1840,7 +1845,9 @@ null |winningComponentConfig|: | |||
| <dd>|leadingBidInfo|'s [=leading bid info/scoring data version=] if it is not null, | |||
| {{undefined}} otherwise | |||
| </dl> | |||
| 1. Let |igAd| be the [=interest group ad=] from |winner|'s [=generated bid/interest group=]'s [=interest group/ads=] whose [=interest group ad/render url=] is |winner|'s [=generated bid/ad descriptor=]'s [=ad descriptor/url=]. | |||
| 1. Let |igAd| be the [=interest group ad=] from |winner|'s [=generated bid/interest group=]'s | |||
| [=interest group/ads=] whose [=interest group ad/render url=] is |winner|'s | |||
There was a problem hiding this comment.
I still think all of these lines should be wrapped properly such that the wrapped line starts exactly under the first character after the number marker on the line above. Actually, you did this below a bit so this is inconsistent. It doesn't really matter, but the entire spec being consistent would be good.
There was a problem hiding this comment.
It's a little unfortunate that it requires indenting the line with tabs plus 1 space to wrap it that way, which does not sound like the best way to indent. I can make the indents in this PR consistent at least, whichever format you'd recommend.
There are some other inconsistencies in the spec as well, such as how a method definition is formatted, maximum length of a line, etc., after more people started contributing to the spec.
Definitely be good to update these, but since it'll be a huge change that might cause some rebase issues (and don't have much bandwidth at this moment), will leave it to a later time when we don't need to modify the spec very often anymore. I can open an issue for this (formatting the spec) if you'd like that.
I remember you have a command line tool that formats specs, do you recommend using it, or I need to manually format the spec?
There was a problem hiding this comment.
It's not a big deal. Consistency is nice, but this is a minor thing ultimately. The tool will not cover this case unfortunately :(
SHA: c277235 Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: c277235 Reason: push, by brusshamilton Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: c277235 Reason: push, by qingxinwu Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Some cleanups came up during another PR's review
Also wrapped some very long lines. Nothing is changed, but just wrapped to new lines.
Preview | Diff