search

bskinn / sphobjinv

Toolkit for manipulation and inspection of Sphinx objects.inv files
https://sphobjinv.readthedocs.io
MIT License
78 stars 9 forks source link

Fix(Ignore) UnicodeDecodeError #226

Closed oscargus closed 2 years ago

oscargus commented 2 years ago

Is the PR a fix or a feature?

Reduces (removes?) the risk of getting UnicodeDecodeErrors. Unknown consequences for the actual search functionality...

Describe the changes in the PR

This may not be the correct way to actually solve it, but it does remove the error and makes suggest work on the objects.inv in question.

Not sure if I should have added a test (the only way I could think of was a smoke test for that particular URL, which I thought didn't really make sense, as it will require a download and the issue may go away from there).

I hope the CHANGELOG entry is close to acceptable.

Does this PR close any issues?

Closes #225

Does the PR change/update the following, if relevant?

bskinn commented 2 years ago

That looks like a great solution! The replace will have some effect on the search/suggest behavior for any objects where a decode error occurs, but I can't imagine it'll noticeably affect user experience.

I would like to add a smoke test for this inventory -- please download a copy of the misbehaving fonttools objects.inv, rename to objects_fonttools.inv, and place in the tests/resource directory. It'll get picked up by the CI.

oscargus commented 2 years ago

(Clearly I didn't look much into adding a test, more thinking about the options.)

Very nice test setup!

bskinn commented 2 years ago

Whoa, well that blew up. Not sure why.

bskinn commented 2 years ago

OH. Because it does a comparison against the inventory housed on the main branch of this repo.

And, that file doesn't exist yet. One sec.

bskinn commented 2 years ago

Oh, interesting -- this issue has revealed a gap in my test suite: I don't have a suggest test that touches all of the inventories in tests/resource.

oscargus commented 2 years ago

Well, it is waiting here for when that is ready. :-) Just let me know if I should do anything on my side.

bskinn commented 2 years ago

Ok! I think main is now ready to accommodate this PR.

Please rebase your working branch for the PR onto my upstream main and push back up to Github. (If you're not sure how to do this, let me know and I can provide steps.)

At this point, the test suite should FAIL.

Then, if you remove the special case for fonttools here, it should 🤞 then make the test suite pass.

oscargus commented 2 years ago

I used a separate commit for the second stage as I wasn't sure if I was doing the right thing (and maybe the error messages from the previous one can be useful?). Let me know if I should squash the commits.

bskinn commented 2 years ago

Excellent -- yeah, you did what I had intended, keeping it as a second commit. It could be squashed, but since this project is single-maintainer and fairly low-velocity, I usually leave the commit history fairly verbose.

Thank you for the issue report and the fix -- I really appreciate it! I'll get v2.2.2 out as soon as I can.

oscargus commented 2 years ago

Great! Happy to be able to assist!