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

Completion of the PID hasTag function #72

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Conversation

mbaitar
Copy link
Contributor

@mbaitar mbaitar commented Nov 30, 2023

When going through the code base, I saw that the function returned a instant panic.

Nothing to worry about since the function wasn't in use.
I've taken it upon myself to complete this small & easy function.

I believe you guys will know where you can use this function.

@perbu
Copy link
Collaborator

perbu commented Nov 30, 2023

Thanks! I think I fixed this in the deadletter actor PR, sorry.

@mbaitar mbaitar closed this Nov 30, 2023
@mbaitar mbaitar reopened this Nov 30, 2023
@mbaitar
Copy link
Contributor Author

mbaitar commented Nov 30, 2023

@perbu I've just tried your hasTag function.
This will work if there are nested tags and you want to find one of the nested tags.

I've created a playground file thats shows you the result if you try to get a tag that is at the end of the pid.ID

It will return false, when it should return true.
I believe you have to split the ID on the pidSeperator and then look if it contains the given tag.

https://go.dev/play/p/OB8So6NyYYR

@perbu
Copy link
Collaborator

perbu commented Nov 30, 2023

I think you're right and I think I introduced a bug. Perhaps you wanna rebase on master?

@mbaitar
Copy link
Contributor Author

mbaitar commented Nov 30, 2023

Rebased on master done!

@mbaitar
Copy link
Contributor Author

mbaitar commented Dec 1, 2023

@anthdm I believe this is ready to be merged to master

@anthdm anthdm merged commit 8dac16e into anthdm:master Dec 1, 2023
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.

4 participants