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 disable String.Chars implementation for Geo objects #110

Closed
wants to merge 1 commit into from

Conversation

Aethelflaed
Copy link

Hello,

I'm using geo_postgis for a web application in which I store GPS coordinates and I would like to change the default string representation of a Geo.Point, as WKT is not well-known enough for end-user.

This small change would allow me to disable the default String.Chars implementation by simply adding the following config:

config :geo, impl_to_string: false

The default behavior of the project is not changed at all.

@aseigo
Copy link
Contributor

aseigo commented Jul 11, 2019

Perhaps it would be nice to allow to over-ride the individual implementations with a module in the config, so you could provide your own, per-type-String.Chars-impl. With macros this should be entirely compile-time, so should not impact performance, but give the needed flexibility ... just a thought :)

@bryanjos
Copy link
Collaborator

Sorry for just getting to this. I think I'm going to push back on this and say to either find a different way to print the string or we should just remove the implementation of String.Chars here altogether.

I think I'm fine with removing it completely

@aseigo
Copy link
Contributor

aseigo commented Aug 9, 2019

I have implemented a configurable version of this feature, which can be seen in my branch here:

https://github.com/aseigo/geo/tree/customizable_string_chars_impls

I will send a push request once my other pending PR is handled ...

@bryanjos
Copy link
Collaborator

I guess for now it may be a good idea to allow it to be configurable. I'll accept a PR for it

@Aethelflaed
Copy link
Author

I'm still quite new to Elixir so I didn't really know what you had in mind (or how exactly to do it), but I like what you've done!

@bryanjos
Copy link
Collaborator

Sorry there are some conflicts here, but if you can resolve those, I'll merge this

@bryanjos
Copy link
Collaborator

I implemented this in master since there was a conflict here.

@bryanjos bryanjos closed this Aug 19, 2019
@Aethelflaed
Copy link
Author

Thank you, sorry for the long reaction :)

I was actually speaking about @aseigo 's implementation, but I guess it could be implemented later anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants