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

Allow to construct constant Locations #16

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

mlafeldt
Copy link
Contributor

No description provided.

@mlafeldt
Copy link
Contributor Author

mlafeldt commented Aug 2, 2022

Hey @srishanbhattarai, mind taking a look? 🙏

@srishanbhattarai
Copy link
Owner

Thanks for sending this in. I haven't looked at this project for a while, could you provide a concrete example that benefits from this change? Bit unsure about the pub fields for long term API stability

@mlafeldt
Copy link
Contributor Author

mlafeldt commented Aug 3, 2022

Sure. One example is this code: https://github.com/mlafeldt/aws-region-nearby/pull/6/files#diff-5e2e69ad103f6876150023c4b80faa31f426770b2689746dafd7643ea6a80113R159

Here, I could turn location() into a const function. Also, I could share city locations between AwsRegion and DenoRegion through constants.

@mlafeldt
Copy link
Contributor Author

mlafeldt commented Aug 3, 2022

If you're concerned about API stability, we might also consider adding a new_const function (or similar) that takes two f64's and returns a const.

@srishanbhattarai
Copy link
Owner

Thanks for the example!

If you're concerned about API stability, we might also consider adding a new_const function (or similar) that takes two f64's and returns a const.

I think this is reasonable solution and I'd be happy to check this in.

@mlafeldt
Copy link
Contributor Author

mlafeldt commented Aug 4, 2022

@srishanbhattarai PTAL :)

@srishanbhattarai
Copy link
Owner

Thanks, LGTM

@srishanbhattarai srishanbhattarai merged commit 03873c8 into srishanbhattarai:master Aug 5, 2022
@mlafeldt mlafeldt deleted the const-location branch August 6, 2022 09:43
@mlafeldt
Copy link
Contributor Author

mlafeldt commented Aug 6, 2022

Thanks for merging. Mind cutting a new release too, so that I can use it? 🙏

@srishanbhattarai
Copy link
Owner

srishanbhattarai commented Aug 7, 2022

Released - https://crates.io/crates/geoutils/0.5.1

Sorry about the delay. Thanks again!

mlafeldt added a commit to mlafeldt/aws-region-nearby that referenced this pull request Aug 31, 2022
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