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

Fix type of PortLinkStatus constants #319

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Conversation

gnuoy
Copy link
Contributor

@gnuoy gnuoy commented Apr 29, 2024

Closes: #318

@stmcginnis
Copy link
Owner

Looking at this now.

Just to note: I edited the description. If you use one of the expected formats, GitHub will automatically link the Issue and the PR so that when the PR merges, it will automatically close the Issue and the two of them will have a reference to each other. Just pointing out for future reference. ;)

@stmcginnis
Copy link
Owner

Good catch!

There are a few definitions where the names needed to be changed do to the spec using the same name for multiple enums or structs. This appears to be one of the cases where EthernetInterface had a LinkStatus definition:

"LinkStatus": {
            "enum": [
                "LinkUp",
                "NoLink",
                "LinkDown"
            ],
            "enumDescriptions": {
                "LinkDown": "No link is detected on this interface, but the interface is connected.",
                "LinkUp": "The link is available for communication on this interface.",
                "NoLink": "No link or connection is detected on this interface."
            },
            "type": "string"
        }

And the newer Port object also added a LinkStatus:

"LinkStatus": {
            "enum": [
                "LinkUp",
                "Starting",
                "Training",
                "LinkDown",
                "NoLink"
            ],
            "enumDescriptions": {
                "LinkDown": "The link on this interface is down.",
                "LinkUp": "This link on this interface is up.",
                "NoLink": "No physical link detected on this interface.",
                "Starting": "This link on this interface is starting.  A physical link has been established, but the port is not able to transfer data.",
                "Training": "This physical link on this interface is training."
            },
            "type": "string"
        }

And to make it even better, they are almost the same thing, but just different enough that they really should be treated separately. It looks like it would be possible to change the original LinkStatus to just have a few more defined types, but that runs the risk of future updates making incompatible changes. So where this has happened in gofish, the newer (usually) type has been prepended with the associated object name, hence in this particular instance the LinkStatus associated with Port becoming PortLinksStatus in the library.

It looks like when this was done, part of updating for that name change was missed, causing the PortLinksStatus values being set as LinkStatus types.

Thanks for finding this and fixing!

@stmcginnis stmcginnis merged commit 9b6d662 into stmcginnis:main Apr 29, 2024
2 checks passed
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.

PortLinkStatus constants appear to be the wrong type
2 participants