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

Fixes #24

Merged
merged 36 commits into from Oct 2, 2023
Merged

Fixes #24

merged 36 commits into from Oct 2, 2023

Conversation

chris-ha458
Copy link
Contributor

Being more strict with cargo clippy and cargo test
looking more into cargo clippy -- -Wclippy::pedantic

As said before, pedantic is just that. Pedantic. There are many false positives and we don't need to fix all of them.
But it still would be valuable to understand why they are false positives and document them if necessary.

@chris-ha458
Copy link
Contributor Author

more drops in accuracy. I should reassess

@chris-ha458 chris-ha458 closed this Oct 2, 2023
@chris-ha458
Copy link
Contributor Author

ah after pulling in 9217701 the accuracy is the same again.

@chris-ha458 chris-ha458 reopened this Oct 2, 2023
@nickspring
Copy link
Owner

Hm... strange but in original lib these changes didn't impact accuracy.

@chris-ha458
Copy link
Contributor Author

considering they are 0.3% difference, we could maybe lay out every single example and compare which is processed which

  1. original (97.1)
  2. fix found in python version (96.8)
  3. canonical fix (95.8)

if you want that prioritized, we can do it, but since these fixes result in same as current main, they can be merged regardless.

Of course if you'd rather stop any other fixes before the root cause for the 1,2,3 discrepency has been found, we can do that as well.

@nickspring
Copy link
Owner

I've checked. All cases have zeros (compare 0 vs 0). I believe we should return this code but eliminate this case (if multibyte usages are 0 and 0 we should compare mess). Please try to return it with this condition.

@chris-ha458
Copy link
Contributor Author

Do you want me to do that on top of here or on a separate PR focused on only this issue?
(I think that would be better)

@nickspring
Copy link
Owner

Hm, you're right it will be better to have it in separate PR.
Concerning this PR - I've sent question about some change which I cannot understand.

@chris-ha458
Copy link
Contributor Author

I cannot see the questions for this PR. Did you leave them as code comments?

Copy link
Owner

@nickspring nickspring left a comment

Choose a reason for hiding this comment

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

There is only one question in my review :)

src/cd.rs Outdated Show resolved Hide resolved
@nickspring nickspring merged commit f6f3bd2 into nickspring:main Oct 2, 2023
3 checks passed
@chris-ha458 chris-ha458 deleted the fixes branch October 2, 2023 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants