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

Add colon_delimiter option and change default behavior #132

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ShuiRuTian
Copy link

@ShuiRuTian ShuiRuTian commented May 29, 2024

#133

Although we did add colon(':') as key value pair separator:
#29

It's more common for user to only treat '=' as the only separator.

And add colon make some edge condition broken, .e.g, some config in .npmrc(it's ini in fact), which has ":" as part of the key.

I add an option in this PR, and change the default behavior to not treat ':' as separator.

I agree it's arguable to change default behavior, what's your opinion?

@zonyitoo
Copy link
Owner

Why not just add an option called key_value_delimiter to let users to specify which delimiter they want to use.

@ShuiRuTian
Copy link
Author

ShuiRuTian commented Jun 4, 2024

Good suggestion, but it seems not that easy. We might need to talk about some design.

  1. What should be the type of key_value_delimiter? For user, it's more reasonable to be Vec<char>, but internally, it's used as &[Option<char>]. Maybe we need to create a new type like ParseOptionInner, when creating parser, we convert the user interface ParseOption to inner data strucutre ParseOptionInner

  2. What should we do if user pass an empty vector? It seems to be an UB.

  3. An error is thrown, what should we do? Feels we need further refactor. Otherwise we need clone, and need to collect memory each time, which is not very good.

error[E0502]: cannot borrow *self as mutable because it is also borrowed as immutable
--> src\lib.rs:1537:9
|
1537 | self.parse_str_until(&self.opt.key_value_delimiter, false)
| ^^^^^---------------^-----------------------------^^^^^^^^
| | | |
| | | immutable borrow occurs here
| | immutable borrow later used by call
| mutable borrow occurs here

  1. To resolve 3, const generic is a resolution, but it causes most of public signature changed. Is this acceptable?

@ShuiRuTian
Copy link
Author

ShuiRuTian commented Jun 4, 2024

Just pushed the changes which uses generic const, what's your thoughts?

Generic const makes default behavior more subtle...

@zonyitoo
Copy link
Owner

zonyitoo commented Jun 4, 2024

That would become a breaking change, because ParseOption is now has a generic type parameter.

Why not we just keep key_value_delimiter to has type Vec<Option<char>> and then add a member function fn set_key_value_delimiter(list: &[char]), that would make everything much easier.

key_value_delimiter must not be an empty list, and it should't has None inside of it. It could be checked in Ini::load_from_*_opt functions.

@zonyitoo
Copy link
Owner

zonyitoo commented Jun 4, 2024

On the other hand, if delimiter is now configurable, whether there are any usecases that wants to have a delimiter with multiple chars, for example, "==". That would require lots of refactors, I would prefer not to do this right now.

@ShuiRuTian
Copy link
Author

ShuiRuTian commented Jun 4, 2024

Why not we just keep key_value_delimiter to has type Vec<Option> and then add a member function fn set_key_value_delimiter(list: &[char]), that would make everything much easier.

The main concern from my side was "self.parse_str_until(&self.opt.key_value_delimiter.clone(), false)", about the perf of clone.

As we could see, it needs clone to satisify the rust borrow constraint, for Vec, clone needs heap allocation. Although I never profile such stuff, but heap allocation is usually concerned kind of slow. And it's why I used const generic, it allows us to use stack rather than heap.

Have no idea, maybe it's fine, anyway, how slow it could be?

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.

2 participants