Skip to content
This repository was archived by the owner on Oct 21, 2025. It is now read-only.

Feat: Change port name delimiter from _ to |#46

Merged
uuqjz merged 3 commits intomainfrom
Delimiter
Oct 6, 2024
Merged

Feat: Change port name delimiter from _ to |#46
uuqjz merged 3 commits intomainfrom
Delimiter

Conversation

@01Parzival10
Copy link
Contributor

No description provided.

@01Parzival10 01Parzival10 requested a review from uuqjz October 2, 2024 14:52
@01Parzival10
Copy link
Contributor Author

I hope thats everything

@uuqjz
Copy link
Contributor

uuqjz commented Oct 2, 2024

If you look at #193 we wanted to use a pipe as delimiter

@uuqjz
Copy link
Contributor

uuqjz commented Oct 2, 2024

I don't quite understand your regex changes. You put ;behind some _but not all. Additionally, shouldn't the new delimiter replace the old one?

@01Parzival10
Copy link
Contributor Author

I don't quite understand your regex changes. You put ;behind some _but not all. Additionally, shouldn't the new delimiter replace the old one?

You do want flow names to be allowed to have "_" in them, right?
| are only allowed in flow names, so in the latter part of the Regex

@uuqjz
Copy link
Contributor

uuqjz commented Oct 3, 2024

I don't quite understand your regex changes. You put ;behind some _but not all. Additionally, shouldn't the new delimiter replace the old one?

You do want flow names to be allowed to have "_" in them, right? | are only allowed in flow names, so in the latter part of the Regex

You mean | should only be allowed between flow names, right?

@01Parzival10
Copy link
Contributor Author

I don't quite understand your regex changes. You put ;behind some _but not all. Additionally, shouldn't the new delimiter replace the old one?

You do want flow names to be allowed to have "_" in them, right? | are only allowed in flow names, so in the latter part of the Regex

You mean | should only be allowed between flow names, right?

Yes.

But you do want "_" to be allowed in a single flow name and also in labels to be compatible with MicroSecEnd? That's why I didn't remove it from the Regex

@uuqjz
Copy link
Contributor

uuqjz commented Oct 3, 2024

But you do want "_" to be allowed in a single flow name and also in labels to be compatible with MicroSecEnd? That's why I didn't remove it from the Regex

Yes id like that

@01Parzival10
Copy link
Contributor Author

But you do want "_" to be allowed in a single flow name and also in labels to be compatible with MicroSecEnd? That's why I didn't remove it from the Regex

Yes id like that

Alright, it should be ready to merge then.

@uuqjz
Copy link
Contributor

uuqjz commented Oct 5, 2024

But you do want "_" to be allowed in a single flow name and also in labels to be compatible with MicroSecEnd? That's why I didn't remove it from the Regex

Yes id like that

Alright, it should be ready to merge then.

I will test the regex some more tomorrow then merge if i don't find an edgecase

Copy link
Contributor

@uuqjz uuqjz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adopt the two comments

@uuqjz uuqjz changed the title Feat: Change port name delimiter from _ to ; Feat: Change port name delimiter from _ to | Oct 6, 2024
Copy link
Contributor

@uuqjz uuqjz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@uuqjz uuqjz merged commit 4167bab into main Oct 6, 2024
@uuqjz uuqjz deleted the Delimiter branch October 6, 2024 12:08
@uuqjz uuqjz linked an issue Oct 8, 2024 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WedEditor delimter needs change

2 participants