chore(api): Make macros with line breaks span a single line only #32510
Conversation
bsmth
requested review from
wbamberg and
teoli2003
and removed request for
a team
github-actions
bot
added
Content:WebAPI
files/en-us/web/javascript/reference/global_objects/map/index.md
Outdated
Show resolved
Hide resolved
| The **`duration`** property of the {{ domxref("AudioBuffer") | ||
| }} interface returns a double representing the duration, in seconds, of the PCM data | ||
| stored in the buffer. | ||
| The **`duration`** property of the {{ domxref("AudioBuffer")}} |
There was a problem hiding this comment.
| The **`duration`** property of the {{ domxref("AudioBuffer")}} | |
| The **`duration`** property of the {{domxref("AudioBuffer")}} |
| pass {{ domxref("BiquadFilterNode") }} (which effectively serves as a bass booster), | ||
| In this example, we grab a media (audio + video) stream from {{domxref("navigator.getUserMedia")}}, feed the media into a {{htmlelement("video")}} | ||
| element to play then mute the audio, but then also feed the audio into a {{domxref("MediaStreamAudioSourceNode")}}. | ||
| Next, we feed this source audio into a low pass {{ domxref("BiquadFilterNode") }} (which effectively serves as a bass booster), | ||
| then a {{domxref("AudioDestinationNode") }}. |
There was a problem hiding this comment.
| then a {{domxref("AudioDestinationNode") }}. | |
| then a {{domxref("AudioDestinationNode")}}. |
| @@ -10,8 +10,7 @@ browser-compat: api.BaseAudioContext.createBuffer | |||
|
|
|||
| The `createBuffer()` method of the {{ domxref("BaseAudioContext") }} | |||
| Interface is used to create a new, empty {{ domxref("AudioBuffer") }} object, which | |||
| can then be populated by data, and played via an {{ domxref("AudioBufferSourceNode") | |||
| }} | |||
| can then be populated by data, and played via an {{ domxref("AudioBufferSourceNode")}}. | |||
There was a problem hiding this comment.
| can then be populated by data, and played via an {{ domxref("AudioBufferSourceNode")}}. | |
| can then be populated by data, and played via an {{domxref("AudioBufferSourceNode")}}. |
| used to play audio data contained within an {{ domxref("AudioBuffer") }} object. | ||
| {{domxref("AudioBuffer") }}s are created using |
There was a problem hiding this comment.
| used to play audio data contained within an {{ domxref("AudioBuffer") }} object. | |
| {{domxref("AudioBuffer") }}s are created using | |
| used to play audio data contained within an {{ domxref("AudioBuffer") }} object. | |
| {{domxref("AudioBuffer")}}s are created using |
| @@ -8,8 +8,7 @@ browser-compat: api.BiquadFilterNode.getFrequencyResponse | |||
|
|
|||
| {{ APIRef("Web Audio API") }} | |||
|
|
|||
| The `getFrequencyResponse()` method of the {{ domxref("BiquadFilterNode") | |||
| }} interface takes the current filtering algorithm's settings and calculates the | |||
| The `getFrequencyResponse()` method of the {{ domxref("BiquadFilterNode")}} interface takes the current filtering algorithm's settings and calculates the | |||
There was a problem hiding this comment.
| The `getFrequencyResponse()` method of the {{ domxref("BiquadFilterNode")}} interface takes the current filtering algorithm's settings and calculates the | |
| The `getFrequencyResponse()` method of the {{domxref("BiquadFilterNode")}} interface takes the current filtering algorithm's settings and calculates the |
I much prefer the second. It is more compact and the whitespace isn't helpful. |
We can have this consistent repo wide by adding a custom rule to our linter: Lines 175 to 179 in 17db3c0 |
|
Thank you for the comments @OnkarRuikar - I agree with all of these suggestions. Shall I handle these in a separate PR? I wanted to keep this one about
Agree with Hamish on this, +1 to the second format. |
|
Shall I handle these in a separate PR? 👍 Yes, that will be easy to fix and review. |
|
I'm not exactly against this change, but:
At least in the two cases you listed, this isn't the case: the linebreak didn't exist before the error was introduced. Personally I think that rather than using grep it would be better to use tools that are less prone to breaking. For example, I just published https://www.npmjs.com/package/query-kumascript, which uses the KS parser to find macro invocations. It seems like this would be more reliable. |
In a similar case for formal syntax changes I did complete yari builds of before and after changes. And compared the build directories using a visual diff tool Meld. Or we could reduce the batch size to 30 and do 7 PRs.
We already have a mechanism in place to lint such custom cases. Following rule in .markdownlint.jsonc should work: {
"name": "linebreaks-in-macros",
"message": "Avoid linebreaks in macro calls",
"searchPattern": "/\\{\\{[^{}]*?\n[^{|]*?\\}\\}/g",
"searchScope": "text",
}The regex may need some tuning.
👍 Ideally this should've been a part of yari tools like the |
|
FWIW I can cope with varying whitespace in macros, but line breaks within them grate. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
Thanks, all. Let me break this one down into smaller PRs. @OnkarRuikar, I feel like 7 is too many PRs, however. 4x55 files with 1LoC changed should be digestible, right? |
|
So, I'm sorry to harp on about this, but splitting it up doesn't actually reduce the reviewer workload or the chance that errors would be introduced. One way to frame this is: what are the expectations on reviewers for a PR like this? It contains 220 pages: is the reviewer expected to:
This is a lot of work, and it's very likely that people will make mistakes.
Yeah, this is what I had in mind. I think it would be great to smooth the edges for this kind of workflow and make it more common for big cleanup PRs.
I'm not so keen on this. We're going to add more complexity to the build, and more fragile regexes, and why? Why is is so important not to break lines in macro calls? I think the answer here is that people want to use grep+regex to find macro invocations, and I'm questioning whether this is the best approach, and whether having more robust tools (e.g. that are based on the KS parser) would work better for us. |
|
Thanks for sharing the concerns! As far as introducing errors, the flaw system should flag broken things and I don't see problems being introduced by converting multi-line invocations to single-line invocations. I've already broken this up into smaller PRs which I think are fine to review by looking at the diffs alone. There's no rush to merge any of them as long as these get tidied up eventually. |
|
FWIW I just reviewed one of these. It was indeed easy to review by inspection. I don't see any problem with converting multi-line invocations to single-line invocations - it is even to my taste. I'd have gone further and removed all arbitrary line breaks. But we should be pretty clear that while this change isn't risky nor does it really add any concrete value other than to people like me who don't "like" arbitrary line breaks :-). I think if we're going to make these kinds of changes we should put in a process to allow automated output comparision so that minimal human review is required to know that the changes are non-breaking. Just my two bits. |
Description
This PR replaces macros that break across multiple lines with single-line invocations.
Motivation
It's impossible to search through the source for a specific pattern without missing edge cases where macros break across multiple lines. The lack of uniformity of this particular case has resulted in missing certain usages very often, such as (
EmbedLiveSamplecalls that contain.pngs), i.e.:It appears that in a lot of cases, this pattern was created through search/replace, and hints at related errors introduced from trying to handle these cases
See
stencil`")}}andpremultipliedAlpha`")}}content/files/en-us/web/api/xrwebgllayer/framebuffer/index.md
Lines 55 to 60 in d77e0bb