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: dot in config name #306

Conversation

CfirTsabari
Copy link
Contributor

@CfirTsabari CfirTsabari commented Mar 27, 2022

Rust std set_extension is either add an extension if none exists, or
edit the current one if such exists.

This caused a mishandling when user is using a name with a dot,
part of the name was treated as an extension, and be overwritten by
the different format extensions.

So manually adding a new dummy extension will cause the current code,
to behave as expected, since it will always overwrite the new dummy
extension and not part of the name.

@matthiasbeyer
Copy link
Collaborator

matthiasbeyer commented Mar 29, 2022

Sorry but I do not understand why this change is necessary rather: How this change works. Could you explain a bit more (prefferably in the commit message)? And maybe provide tests for both bad cases and good cases? 🤔

Rust std `set_extension` is either add an extension if none exists, or
edit the current one if such exists.

This caused a mishandling when user is using a name with a dot,
part of the name was treated as an extension, and be overwritten by
the different format extensions.

So manually *adding* a new dummy extension will cause the current code,
to behave as expected, since it will always overwrite the new dummy
extension and not part of the name.
@CfirTsabari CfirTsabari force-pushed the cfirtsabari/doesn-t-work-with-dot-101 branch from 60c299d to 34ea07a Compare April 10, 2022 18:42
@CfirTsabari
Copy link
Contributor Author

CfirTsabari commented Apr 10, 2022

@matthiasbeyer hi,
changed the commit message.
regarding the tests I have added a case for the bad case but the good cases is all of the other existing tests.

@matthiasbeyer matthiasbeyer merged commit 44a1216 into mehcode:master Apr 10, 2022
@matthiasbeyer
Copy link
Collaborator

Thanks! I really like the commit message, thanks for improving it! ❤️ 🎉

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