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

define propertynames() to be consistent with getproperty() #1289

Merged
merged 8 commits into from
Jan 27, 2025

Conversation

aplavin
Copy link
Member

@aplavin aplavin commented Dec 8, 2024

No description provided.

@aplavin
Copy link
Member Author

aplavin commented Jan 5, 2025

monthly bump... nightly failures are irrelevant

Co-authored-by: Neven Sajko <[email protected]>
@aplavin aplavin requested a review from nsajko January 6, 2025 14:50
@nsajko
Copy link
Contributor

nsajko commented Jan 6, 2025

Wait, is data a private or public property? If it's public, it should be included in propertynames(...) in any case. If it's private, you might still want to include it when the boolean argument is true.

@nsajko
Copy link
Contributor

nsajko commented Jan 6, 2025

I guess the maintainers need to decide whether data is a public property.

@nsajko
Copy link
Contributor

nsajko commented Jan 6, 2025

An issue with this PR is that it only overloads propertynames when getproperty is overloaded, leading to inconsistency. :data ∉ propertynames(SVector(1, 2, 3, 4)), but :data ∈ propertynames(SVector(1, 2, 3, 4, 5)). Either have both calls include :data or neither IMO.

@aplavin
Copy link
Member Author

aplavin commented Jan 6, 2025

Indeed, nice catch! Fixed :data handling to be consistent.

aplavin and others added 2 commits January 6, 2025 14:56
@aplavin
Copy link
Member Author

aplavin commented Jan 27, 2025

gentle bump...

Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

LGTM. Could you just bump patch version? I will merge and tag.

@aplavin
Copy link
Member Author

aplavin commented Jan 27, 2025

done

@mateuszbaran mateuszbaran merged commit 1949ebe into master Jan 27, 2025
21 of 26 checks passed
@mateuszbaran mateuszbaran deleted the propnames branch January 27, 2025 18:57
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.

3 participants