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: Improve type prefix and suffix extraction #13

Merged
merged 7 commits into from
Jun 16, 2024

Conversation

iron3oxide
Copy link
Contributor

Refactored the logic in get_prefix_and_suffix function to improve type prefix and suffix extraction. Now using rsplit method to accurately extract the prefix and suffix from the input string.

Closes #12

Refactored the logic in `get_prefix_and_suffix` function to improve type
prefix and suffix extraction. Now using `rsplit` method to accurately extract
the prefix and suffix from the input string. 

Closes akhundMurad#12
fix: Improve type prefix and suffix extraction
@akhundMurad
Copy link
Owner

Hello @iron3oxide
Your changes broke the TypeID spec validation.
Please try to resolve them and add additional tests to your case.
Thanks for your contribution!

@iron3oxide
Copy link
Contributor Author

Done! My pleasure

typeid/typeid.py Show resolved Hide resolved
typeid/typeid.py Show resolved Hide resolved
@iron3oxide
Copy link
Contributor Author

@khasizadaj I am replying here because it is easier to follow the conversation this way. I don't quite understand your suggested changes.

To preface this: In my opinion, get_prefix_and_suffix() should aim to do just that: extract the prefix and suffix candidates from the given string. A special case where it is preferable to raise an error early is a single leading underscore in the given string.

After that it is up to the TypeID class to correctly flag the given prefix and suffix as invalid; which it does, as the tested string in the test you mentioned contains a valid prefix according to the TypeID spec, but no valid suffix. A string such as "___test_invalid_00041061050r3gg28a1c60t3gf" is also correctly flagged as containing an invalid prefix. Could you give me an example string where the changes suggested in this PR produce an unexpected or wrong outcome?

@khasizadaj
Copy link

Thanks for the clarification. I was thinking that it could be useful to throw exception before that as well, but now I understand your POV as well. We can disregard my comment.

@akhundMurad
Copy link
Owner

@iron3oxide @khasizadaj
may I resolve the conversation above?

@iron3oxide
Copy link
Contributor Author

Yes, I think it's ready to be merged

@akhundMurad
Copy link
Owner

@iron3oxide @khasizadaj
thanks, guys!
I will include it in the next release.

@khasizadaj
Copy link

Yes, it can be closed! Thanks a lot, @iron3oxide.

@akhundMurad akhundMurad merged commit 850fe49 into akhundMurad:main Jun 16, 2024
3 checks passed
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.

TypeID.from_string() validates incorrectly
3 participants