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

refactor(loops): Clean up some loops #3362

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Zearin
Copy link
Contributor

@Zearin Zearin commented Jul 11, 2024

Lots of small tweaks. Usually to make loops easier to read, and sometimes to replace a loop with something that is (hopefully) easier or faster to read.

@Zearin
Copy link
Contributor Author

Zearin commented Jul 15, 2024

@zachleat Beep! :)

@zachleat zachleat self-requested a review as a code owner August 8, 2024 17:17
@@ -212,7 +212,7 @@ class Template extends TemplateContent {
let results = await Promise.all(promises);

permalinkValue = {};
for (let j = 0, k = keys.length; j < k; j++) {
for (let j = 0; j < keys.length; j++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Extracting the length into a variable used to be a performance improvement thing. Is that irrelevant now with newer browser versions?

continue;
}

for (let item of this.collectionsData[collectionName]) {
for (let item of collectionData) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Is Line 674 still working as expected? Is there a test for it?


let matches = paths.filter((path) => {
return !this.extensionMap.hasEngine(path);
});
return matches;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We might as well return it inline :)

Copy link
Member

@zachleat zachleat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just revert those two blocks and I’m happy!

for (let path of this.extensionMap.getPassthroughCopyGlobs(this.inputDir)) {
paths.add(path);
}
let paths = new Set(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with almost all of the style changes here except the .concat ones. I prefer the old method, sorry :-/

uniqueIgnores.add(ignore);
}
let uniqueIgnores = new Set(
this.fileIgnores
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one

@zachleat zachleat added the needs-changes A pull request that requires additional changes before it can be merged. label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-changes A pull request that requires additional changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants