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 to Issue #5462 #5466

Merged
merged 1 commit into from
Nov 14, 2021
Merged

Fix to Issue #5462 #5466

merged 1 commit into from
Nov 14, 2021

Conversation

dhowe
Copy link
Contributor

@dhowe dhowe commented Oct 29, 2021 •
edited
Loading

fix to #5462 - remove index check from p5.Font.isSpace() and fix broken tests

@dhowe dhowe requested a review from outofambit October 29, 2021 10:08
@dhowe dhowe mentioned this pull request Oct 29, 2021
Closed
17 tasks
@limzykenneth
Copy link
Member

I'm happy to just remove the commented out parts of this PR entirely since we will still have a record of changes in git. In any case we can tentatively merge this for now and keep an eye out if anything somehow doesn't work after this.

@dhowe
Copy link
Contributor Author

dhowe commented Oct 31, 2021

it occurs to me that we may need to handle other types of whitespace in that function, let me do a few more tests before we merge

@outofambit outofambit removed their request for review November 1, 2021 15:50
@outofambit
Copy link
Contributor

sounds good! @dhowe would you re-request a review when this is ready to look at again?

@dhowe
Copy link
Contributor Author

dhowe commented Nov 12, 2021

ok with spaces and tabs, can be merged as is

@limzykenneth
Copy link
Member

@dhowe I'm not seeing new commits? Or there's no new commits for now?

@dhowe
Copy link
Contributor Author

dhowe commented Nov 13, 2021

no new commits for now, this should be working as expected

outofambit
outofambit approved these changes Nov 14, 2021
View reviewed changes
Copy link
Contributor

@outofambit outofambit left a comment

Choose a reason for hiding this comment

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

looks good to me!

were the console.logs in the tests here causing problems, or is that cleanup?

@dhowe
Copy link
Contributor Author

dhowe commented Nov 14, 2021

were the console.logs in the tests here causing problems, or is that cleanup?

just cleanup

@limzykenneth
Copy link
Member

I'm going to merge this then. If any problems with it pops up we'll know where to look. Thanks @dhowe

@limzykenneth limzykenneth merged commit 8fddc40 into main Nov 14, 2021
@limzykenneth limzykenneth deleted the Issue#5462 branch November 14, 2021 13:55
This was referenced Feb 3, 2022
Merged
Merged
Open
Open
This was referenced Mar 7, 2022
Open
Open
Merged
Open
Open
@renovate renovate bot mentioned this pull request Nov 20, 2022
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@outofambit outofambit outofambit approved these changes

@limzykenneth limzykenneth Awaiting requested review from limzykenneth

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
@dhowe @limzykenneth @outofambit