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.

Sign up for GitHub

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

Jump to bottom

fix: Added process to remove last empty row for examples in function references. #561

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

fix: Added process to remove last empty row for examples in function references. #561

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/scripts/builders/reference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ const correctRelativeLinksToExampleAssets = (
return content;
}
return Array.isArray(content)
? content.map((ex) => ex.replaceAll("assets/", "/assets/"))
? content.map((ex) => ex.replaceAll("assets/", "/assets/").replaceAll(/\n$/g, ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding this! Two thoughts:

  • Do you think we should also add it on the line below in the case when the content is just a string?
  • To make things easier for other contributors coming into this in the future, maybe we should make a new function like removeTrailingNewlines() to do this functionality and then call it in places where we call correctRelativeLinksToExampleAssets()? It might be unexpected, based on the name of the function, that it would include this functionality. Alternatively, maybe we can rename this to something more general, like cleanUpExampleContents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you pointed out, you are right, the createRelativeLinksToExampleAssets function is indeed for modifying the links.
This was not the appropriate part of this response...

I like the idea of either removeTrailingNewlines() or cleanUpExampleContents().

( Or, I still think the policy should be to fix it in the p5.js source code... )

Copy link
Collaborator

@davepagurek davepagurek Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think fixing it in the p5 repo also works, although we'd maybe want to add a check in CI that there aren't trailing newlines, because I can imagine it being pretty easy to miss in code review otherwise.

But in the mean time, renaming this function or putting this change in another function that's called alongside this one is also fine by me! Thanks for finding the cause and for making these changes so far 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you too.
I am happy to help improve p5.js!

: content.replaceAll("assets/", "/assets/");
};

Expand Down