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

LocationAttachment fix #395

Merged
merged 11 commits into from
Feb 19, 2019
Merged

LocationAttachment fix #395

merged 11 commits into from
Feb 19, 2019

Conversation

darylkell
Copy link
Contributor

Fix for locations where address is provided instead of GPS co-ords (issue #392)

  • Add self.location attribute to class LocationAttachment to house address string
  • Fix for graphql_to_extensible_attachment(a) to include location attribute for addresses, while maintaining current lat/long attributes

Fix for locations where address is provided instead of GPS co-ords
@darylkell
Copy link
Contributor Author

I can see the black lint check failed, and I have seen on a previous PR that this is requested to be fixed by referring to 'CONTRIBUTING.rst' which to be honest is not a guide at all.

As mentioned 20 days ago by madsmtm in a PR, the use of flint has indeed increased the initial difficulty of manual installation (users have to learn a new tool) and unfortunately I can't help any more than provide what I have in this initial PR.

Anyway, I hope it helps.

@kapi2289
Copy link
Contributor

Please run this in your fbchat directory

$ pip install -U black
$ black .

I would change the location attribute name to address.
And please mention in the attribute docs that available are only the coordinates OR only the address

Copy link
Contributor Author

@darylkell darylkell left a comment

Choose a reason for hiding this comment

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

LocationAttachment attribute 'address' renamed.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm sorry you're having trouble with flit and black, they're mostly tools to increase my own productivity, but perhaps just as much my own little "political push" towards a better Python development infrastructure 😉. Anyhow, if you'd like to discuss the decision further, I truly encourage you to open an issue about it, this project is probably seen by a lot of beginners, and I'd hate to loose valueable contributions! 😊 (Also, regarding CONTRIBUTING.rst, I'll gladly admit that I'm terrible at writing documentation, if you've got suggestions for improval, then I'm open for criticism!)

Thanks for the PR though, I have a few small nitpicks, but otherwise it looks fine 👍

fbchat/graphql.py Outdated Show resolved Hide resolved
fbchat/graphql.py Outdated Show resolved Hide resolved
fbchat/graphql.py Outdated Show resolved Hide resolved
fbchat/models.py Show resolved Hide resolved
@darylkell
Copy link
Contributor Author

I'm happy for you to do whatever you like with the code I've presented to make it viable for your project. I've had a crack at using GitHub to fork/push request etc and learned a bit trying to contribute to FBchat - and I appreciate you bearing with me along the way.
That said, I'm not bothered by having a successful PR contribution next to my name, just wanted to offer a means to improve the API so perhaps you can find some use from what I've added here.

fbchat/graphql.py Outdated Show resolved Hide resolved
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

LGMT, thanks for the help, i appreciate the effort 👍

@madsmtm madsmtm merged commit caa2ecd into fbchat-dev:master Feb 19, 2019
@chelsybradley15 chelsybradley15 linked an issue Mar 25, 2023 that may be closed by this pull request
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