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

Prevent null issues when strings are unequal #290

Merged
merged 12 commits into from
Mar 31, 2024
Merged

Conversation

elken
Copy link
Contributor

@elken elken commented Mar 9, 2024

In the case where s2 is larger than s1, this errors because the index is out of range.

A good testcase is the below

(apheleia--align-point 
 "       <div class=\"left-[40rem] fixed inset-y-0 right-0 z-0 hidden lg:block xl:left-[50rem]\">\n  <svg\n"
 "<div class=\"left-[40rem] fixed inset-y-0 right-0 z-0 hidden lg:block xl:left-[50rem]\">\n <svg"
6)

If I've implemented the indexing wrong, do let me know but this seems to work just fine now for mix (the formatter that triggered this)

In the case where s2 is larger than s1, this errors because the index is
out of range.
---

no changelog update needed
@raxod502
Copy link
Member

I'm going to fix up this unit testing framework a bit to be more usable, add some additional test cases for the string alignment function, and then merge. It might take a bit but I'll get it done.

@elken
Copy link
Contributor Author

elken commented Mar 10, 2024

Sounds good!

@raxod502
Copy link
Member

Okay, so I wrote some tests, and the test framework is very pretty now, but the tests are not passing... problem is, I'm pretty sure the tests are right and actually the string alignment algorithm has been wrong since I wrote it in 2019. Unfortunately I have no clue how it works anymore. So I will be figuring that out and fixing it, I guess.

I bet that will solve #2...

image

@raxod502
Copy link
Member

Aha, it WAS wrong!! And now it's fixed, I can't believe we've all (myself included) been living with the core algorithm of this package just being... wrong, for so long. Never again, I learned my lesson and will write tests.

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

@raxod502
Copy link
Member

Sorry it took so long but I hope you'll agree the result is worth it!

@raxod502 raxod502 merged commit b776ed9 into radian-software:main Mar 31, 2024
4 checks passed
@elken elken deleted the main branch March 31, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants