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

Attempt to remove the subject_transform_dest struct key #861

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

ripienaar
Copy link
Collaborator

No description provided.

@ripienaar ripienaar requested a review from jnmoyne September 19, 2023 09:03
} else if source.FilterSubject != "" && source.SubjectTransformDest != "" {
edge.Label(source.FilterSubject + " to " + source.SubjectTransformDest)
} else if len(source.SubjectTransforms) == 1 {
edge.Label(source.SubjectTransforms[0].Source + " to " + source.SubjectTransforms[0].Destination)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems we never did anything here if SubjectTransforms were set at all? So I limited the change to just if its exactly 1

Copy link
Contributor

Choose a reason for hiding this comment

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

I think not handling SubjectTransforms here was simply an omission, so no need to limit it to just == 1, it should just be if > 1

Copy link
Collaborator Author

@ripienaar ripienaar Sep 19, 2023

Choose a reason for hiding this comment

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

not convinced that would work, because here the label pairs are to->dest and it needs to handle many. Just using a >1 here would not look good or be complete

I think I will leave it as is for release, we can fix later to handle multiple better

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put something like "multiple" in the filter and destinations columns if there's more than 1 in SubjectTransforms?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then find a way to display more than one in the table format later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These edges are here are used to produce a dot graph, not for the table, its not something to fix today I am afraid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2023-09-19 at 18 57 22

Fixing this to show multiple sources etc, possible of course but more surgery than I want now. I will release 0.1.0, we can do 0.1.1 not long in the future to add these

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ripienaar ripienaar force-pushed the subject_transform_dest branch 3 times, most recently from e405e4e to 2d1d369 Compare September 19, 2023 11:52
@ripienaar ripienaar force-pushed the subject_transform_dest branch from 2d1d369 to 8de8703 Compare September 19, 2023 14:27
@ripienaar ripienaar mentioned this pull request Sep 19, 2023
Copy link
Contributor

@jnmoyne jnmoyne left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jnmoyne jnmoyne left a comment

Choose a reason for hiding this comment

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

LGTM

@ripienaar ripienaar merged commit 4716669 into nats-io:main Sep 19, 2023
2 checks passed
@ripienaar ripienaar deleted the subject_transform_dest branch September 19, 2023 17:34
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