Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ignore if not exists #176

Merged
merged 1 commit into from
Dec 22, 2023
Merged

ignore if not exists #176

merged 1 commit into from
Dec 22, 2023

Conversation

burrbull
Copy link
Member

@burrbull burrbull commented Oct 26, 2023

Closes #67

@burrbull burrbull marked this pull request as ready for review October 26, 2023 11:00
@adamgreig
Copy link
Member

I like the idea, but I'm not totally sure about the syntax. Maybe it's OK, but I worry it will look too confusing, or might be mixed up for a real start of a rule. Could you add it to the README too as an example?

Maybe another option is a "_missing_ok: True" field. It can't be used everywhere, but I think it could be used anywhere that we want to patch something and it might not exist.

@burrbull
Copy link
Member Author

Maybe another option is a "_missing_ok: True" field. It can't be used everywhere, but I think it could be used anywhere that we want to patch something and it might not exist.

Adding option is not ok because there is not always an a hash, sometimes there is also an array or nothing.

@burrbull
Copy link
Member Author

burrbull commented Nov 11, 2023

Yet one argument against _missing_ok is yaml_includes.
Now we merge entries with identical names during include (if you ask me it is now the biggest issue of current system as it changes sequence of patch rules). The best solution would be not merge them at all. But this is not easy to implement. Although I have one idea.

@burrbull burrbull mentioned this pull request Nov 11, 2023
@burrbull burrbull force-pushed the not-exists branch 2 times, most recently from 1f09b9e to 4988a17 Compare November 30, 2023 15:53
@burrbull burrbull force-pushed the not-exists branch 2 times, most recently from c9aa6c6 to cc3b963 Compare December 19, 2023 07:39
@burrbull
Copy link
Member Author

cc @Emilgardis @kossnikita do you have any thoughts/suggestions?

@kossnikita
Copy link
Contributor

I don't like _missing_ok. It is not clear where it should be used and how to disable it. I would like to see it as an argument. But in the case when you need to allow this behavior for a specific patch, I would prefer the original option proposed by @burrbull .

@burrbull
Copy link
Member Author

burrbull commented Dec 19, 2023

What should it be? Suffix or prefix? And what sequence of symbols is better (not allowed in svd names and glob syntax, easy for search)?

@burrbull
Copy link
Member Author

Tilde maybe?
"?~SPEC"
Looks clearer.

@kossnikita
Copy link
Contributor

kossnikita commented Dec 19, 2023

Is it possible to do this regex-like [abc] or [a,b,c]?

"[TIM[18],TIM20]":

or

"(TIM[18],TIM20)?":

EDIT: It is more correct to use * rather than ? if do it like in regex

@burrbull
Copy link
Member Author

Is it possible to do this regex-like [abc] or [a,b,c]?

"[TIM[18],TIM20]":

or

"(TIM[18],TIM20)?":

EDIT: It is more correct to use * rather than ? if do it like in regex

It is possible to check both prefix and suffix, but I don't like it:

  1. We already use a lot of ? and * at the end of spec when matching arrays and similar things. This is why I prefer prefix then suffix.
  2. This extended spec can be passed to is_match accidentally. svdtools should stop with error in such situation. So it should not be correct match syntax.
  3. Matches uses glob syntax, not regex. Less powerful, but less messy. Mixing does not look a good idea.

@kossnikita
Copy link
Contributor

Matches uses glob syntax, not regex.

Sorry, I'm new and don't know that much about crates. This clarifies a lot for me. Then the prefix seems to be the best solution. ?~ or ?@ doesn't matter to me.

@burrbull
Copy link
Member Author

Merging.

@burrbull burrbull merged commit dca37ea into master Dec 22, 2023
@burrbull burrbull deleted the not-exists branch December 22, 2023 06:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore missing registers
3 participants