Skip to content

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
1 check passed
@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 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.

WedEditor delimter needs change
2 participants