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

Make SimplifyVw algorithm naming consistent. #957

Merged
merged 4 commits into from
Jan 19, 2023
Merged

Make SimplifyVw algorithm naming consistent. #957

merged 4 commits into from
Jan 19, 2023

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Dec 18, 2022

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Fixes: #575

Changed:

  • SimplifyVW -> SimplifyVw
  • SimplifyVWPreserve -> SimplifyVwPreserve
  • simplifyvw -> simplify_vw

Unchanged:

  • SimplifyVwIdx

Fixes: #575

Changed:

* `SimplifyVW` -> `SimplifyVw`
* `SimplifyVWPreserve` -> `SimplifyVwPreserve`
* `simplifyvw` -> `simplify_vw`

Unchanged:

* `SimplifyVwIdx`
Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

Sorry that this (and all your other simple PR's) sat here for so long. I guess I lost track of them!

I agree that these new names are more idiomatic.

It'd be nicer to deprecate this in a non-breaking release. Do you have time to make that change?

@frewsxcv
Copy link
Member Author

frewsxcv commented Jan 9, 2023

Sorry that this (and all your other simple PR's) sat here for so long. I guess I lost track of them!

I agree that these new names are more idiomatic.

It'd be nicer to deprecate this in a non-breaking release. Do you have time to make that change?

I do, but I don't think it's worth the maintenance burden for us. We have deprecated features lingering in the code and it's extra effort for us to remember to remove them, and this is a trivial one or two character change for end users. I don't feel strongly about this though.

@michaelkirk
Copy link
Member

I think it's nicer for our users to prefer deprecation unless there is some abnormal circumstances which would make it unrealistic to apply them (granted that sometime's it's just not practical).

It's unfortunate that tracking deprecations would be an inordinate burden. Granted it's always going to be more than zero work - they are something a maintainer can do (hence, a burden), for the sake of a hopefully nicer experience for users.

I typically scan the deprecations before doing a release, and delete any "old" ones: https://github.com/georust/geo/pull/892/files

Maybe there's a better system by which we could track them?

I agree it's a small thing in this particular case. Actually, I think the compiler will already suggest the appropriate rename for the trait (but seemingly not the method, surprisingly). So I'm fine with merging this as is, but let me know if you have any thoughts on a better way to track deprecations for the future.

@frewsxcv
Copy link
Member Author

So I'm fine with merging this as is, but let me know if you have any thoughts on a better way to track deprecations for the future.

It's too bad there's not an easy way to set reminders in Discord like there is in Slack. So at least we'd get some sort of notification in the future that we can remove deprecated things.

@frewsxcv
Copy link
Member Author

bors r=michaelkirk

@bors
Copy link
Contributor

bors bot commented Jan 19, 2023

Build succeeded:

@bors bors bot merged commit 28b8b29 into main Jan 19, 2023
@bors bors bot deleted the simplifyvw branch January 19, 2023 00:44
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.

Make capitalization consistent for for Visvalingam-Whyatt simplification algorithms
2 participants