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

[fix] Logic error with quotes order #12

Closed
wants to merge 2 commits into from

Conversation

trueberryless
Copy link
Contributor

@trueberryless trueberryless commented Jan 15, 2025

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

In this line there is a small logic error, which went unnoticed because there are currently no tests for setting either smart or straight.

Detailed context:

Let's say these are the current vars:

markers = ['“”', '‘’', '«»']
stack = [{ "“", "smart", "open" }]

And now look at this line:

expected = markers[(stack.length + 1) % markers.length]

We take the length of the stack, which is 1, add 1 to it, which makes 2 and module (%) it with the length of the markers, which is 3, in order to make sure, that we don't get any out of range exception.

We expect that the expected variable is set to the first item in markers, namely '“”' because this is the very first quote in the paragraph, as you can see: The stack only has the one item, no other quotes have been opened.

However, the item with index 2 is the third item, which leads the plugin to say, that the first quote is wrong, it should be the third quote...

Basically, we are ALWAYS 2 indecies ahead of the actual marker we want to be in the var expected.

This went unnoticed until now, because all tests only ever have max 2 entries in the markers array, which by coincidence doesn't affect the output, because the module % resets it to 0, so expected gets set to the right item / right index.

In order to fix this behaviour, we have to subtract 1 from the stack length, so we are exactly at the index we want to be, like this:

expected = markers[(stack.length - 1) % markers.length]

If you have any further questions explaining the issue, please let me know!

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Jan 15, 2025

This comment has been minimized.

@trueberryless
Copy link
Contributor Author

As you can see, the tests I wrote in #11 are now passing (that's also the reason I also included them in this PR)

Copy link
Contributor Author

@trueberryless trueberryless left a comment

Choose a reason for hiding this comment

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

I left some suggestions, which should be committed before merging...

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Jan 15, 2025
@github-actions github-actions bot removed the 👋 phase/new Post is being triaged automatically label Jan 15, 2025
@wooorm
Copy link
Member

wooorm commented Jan 15, 2025

Hi! Thanks!

The template includes <!--do not edit: pr--> — you can leave that. It’s a directive for the bots. Without it they don’t know what you are doing.

I like narrowing the problem down as much as possible, which results in a test, and then finding the smallest possible solution, which often impacts that test again.
So: thanks for your work! Boiled it down to a small patch myself.

@wooorm
Copy link
Member

wooorm commented Jan 15, 2025

released in 6.0.1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

2 participants