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

Rename Maps to PdoMaps and Map to PdoMap #431

Merged
merged 2 commits into from
May 15, 2024

Conversation

sveinse
Copy link
Contributor

@sveinse sveinse commented May 14, 2024

This PR renames the class canopen.pdo.base.Map to canopen.pdo.base.PdoMap and correspondingly .Map to PdoMap in line with the similar change as PR #368. The names PdoMap() and PdoMaps() with respect to upper case "PDO" is chosen to be consistent with how Sdo* have been named.

Compatibility entries have been added, so this won't break old code.

@erlend-aasland
Copy link
Contributor

Why not keep the old names as aliases coupled with DeprecationWarnings if used? That way you'll give users a chance to update their code before actually performing the breaking change.

@sveinse
Copy link
Contributor Author

sveinse commented May 14, 2024

Mainly because there is no DeprecatedWarnings on the other change (#368) so I figured that wasn't wanted behavior. Also note that this PR doesn't break anything.

We can add DeprecatedWarnings if that is what we want to have.

@erlend-aasland
Copy link
Contributor

Ah, I missed that you already provided aliases for the old variant; nice. I would consider adding .. versionchanged directives in the docs to inform users of the change, though.

Mainly because there is no DeprecatedWarnings on the other change [...]

👍

@sveinse
Copy link
Contributor Author

sveinse commented May 14, 2024

I would consider adding .. versionchanged directives in the docs to inform users of the change, though.

While I agree, I see there is no such framework established in the existing docs. Or any change log for that matter. I'm not sure this simple PR should establish that precedence thou.

@erlend-aasland
Copy link
Contributor

Fair enough!

canopen/pdo/base.py Outdated Show resolved Hide resolved
@acolomb
Copy link
Collaborator

acolomb commented May 15, 2024

@sveinse Could you please check if there are any references to the old names in the docs / examples?

@sveinse
Copy link
Contributor Author

sveinse commented May 15, 2024

@sveinse Could you please check if there are any references to the old names in the docs / examples?

Yes, I did not find any more that the ones that are included in the PR. I just double checked. 👌

@acolomb acolomb merged commit d38045f into christiansandberg:master May 15, 2024
1 check passed
@sveinse sveinse deleted the feature-rename-pdomap branch May 15, 2024 21:21
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.

None yet

3 participants