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.

Sign up for GitHub

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

Jump to bottom

fix: /nextmeetup command #30

Merged
merged 2 commits into from
Aug 28, 2022
Merged

fix: /nextmeetup command #30

merged 2 commits into from
Aug 28, 2022

Conversation

ditsuke
Copy link
Contributor

@ditsuke ditsuke commented Aug 7, 2022 •
edited
Loading

What

Fixes the /nextmeetup command using an internal meetup.com API.
Also adds autopep8 for code formatting.

Why

Command was broken

The command in action

image

Disclaimer

This is a Stacked PR stacked atop #27.
Please review in tandem and merge the PRs, without the squash or rebase options, in sequence.
For easier testing please use the latest PR.

@ditsuke
Copy link
Contributor Author

ditsuke commented Aug 7, 2022 •
edited
Loading

/cc @realslimshanky @zora89 @yednapg

@realslimshanky
Copy link
Owner

realslimshanky commented Aug 7, 2022 •
edited
Loading

@ditsuke Pip package is not updated since 2016 and I see the last change in the meetup-api is done on 2019. It's better to install using the GitHub source pip install git+https://github.com/pferate/meetup-api.git@master

Could you please update your PR accordingly and test.

@ditsuke
Copy link
Contributor Author

ditsuke commented Aug 7, 2022 •
edited
Loading

@ditsuke Pip package is not updated since 2016 and I see the last change in the meetup-api is done on 2019. It's better to install using the GitHub source pip install git+https://github.com/pferate/meetup-api.git@master

Could you please update your PR accordingly and test.

@realslimshanky I'm not using a package for the api. The meetup_api import is for the meetup_api.py file

@realslimshanky
Copy link
Owner

@ditsuke Okay, I missed that file. Thanks.

@realslimshanky
Copy link
Owner

realslimshanky commented Aug 8, 2022 •
edited
Loading

@ditsuke I read up on stacked pull requests and I think you missed an essential point while creating pull requests i.e. stacking. Instead of creating pull requests on top of another existing branch (associated with a pull request), you've made all the pull requests to be merged directly to the master branch. So, they're not stacked at all.
#26 contains dependency manager change and ci fix
#27 contains pytz upgrade + changes in #26 are visible
#30 (i.e. this PR) contains /nextmeetup command fix + changes in #26 + changes in #27 are visible

This can be resolved by making sure #26 merged to master, #27 merged to the branch used to create #26 and #30 merged to the branch used to create #27. You see the issue here, we're increasing the complexity of PR reviews. Now, it can be useful if we're trying to solve 1 Big Problem in small chunks that are dependent on one another. But in this case #26, #27, and #30 can be completely independent.

By default, it's best to create one issue per problem/feature for the repo and one PR for each issue. In case the issue is too big, dissect it into smaller issues and make sure to have only one PR per issue. This is the best way to tackle individual problems without worrying about other independent issues. And this way, the PR review will be easier.

What do you think about this? Which approach do you think solves our problem in the case of these 3 PRs?

@engagepy
Copy link
Contributor

engagepy commented Aug 9, 2022 via email

@progmatic-99
Copy link
Contributor

Yes, I agree with @realslimshanky that solving 1 issue per PR makes more sense.

@ditsuke
Copy link
Contributor Author

ditsuke commented Aug 9, 2022

@ditsuke I read up on stacked pull requests and I think you missed an essential point while creating pull requests i.e. stacking. Instead of creating pull requests on top of another existing branch (associated with a pull request), you've made all the pull requests to be merged directly to the master branch. So, they're not stacked at all. #26 contains dependency manager change and ci fix #27 contains pytz upgrade + changes in #26 are visible #30 (i.e. this PR) contains /nextmeetup command fix + changes in #26 + changes in #27 are visible

By default, it's best to create one issue per problem/feature for the repo and one PR for each issue. In case the issue is too big, dissect it into smaller issues and make sure to have only one PR per issue. This is the best way to tackle individual problems without worrying about other independent issues. And this way, the PR review will be easier.

What do you think about this? Which approach do you think solves our problem in the case of these 3 PRs?

@realslimshanky thanks, I totally missed that point around stacked PRs! However with that approach to stacking PRs, I would be unable to use them at all since I can't create branches on this repo. I also agree with your point on a PR/issue or isolated subject, which is why I was motivated to use stacking in the first place.

This can be resolved by making sure #26 merged to master, #27 merged to the branch used to create #26 and #30 merged to the branch used to create #27. You see the issue here, we're increasing the complexity of PR reviews. Now, it can be useful if we're trying to solve 1 Big Problem in small chunks that are dependent on one another. But in this case #26, #27, and #30 can be completely independent.

I agree again! That being the case, since I have dependency changes in #27 and #30, they are in a way dependent on #26, or at least not having them follow a linear sequence of commits affects my DX because I then have to change dependencies in the original requirements.txt which I've removed in #26. This would mean merging #27 or #30 first would cause merge conflicts in #26, and conversely merging #26 first would cause conflicts in the other 2 PRs. In any case, I think I can decouple these PRs with some effort on my end. Since it looks like this PR gets through first I'll start here.

@realslimshanky
Copy link
Owner

@ditsuke great. Let’s start with cleaning up the PR so that none of them have overlapping changes. Thanks.

@ditsuke
fix: Upcoming event from meetup.com
e84a288 
Adds support for fetching meetup info from an unofficial API
@ditsuke ditsuke force-pushed the fix/next-event-meetup branch from fa33e84 to e84a288 Compare August 9, 2022 15:34
@ditsuke
Copy link
Contributor Author

ditsuke commented Aug 9, 2022

@realslimshanky I've decoupled this PR. Please take a look

realslimshanky
realslimshanky reviewed Aug 13, 2022
View reviewed changes
pydelhi.py Outdated
date_time = date_time.strftime('%Y-%m-%d %H:%M:%S %Z%z')
is_online = r['is_online']

# ? is it event possible to get the venue etc from the unoffical API? possibly not
Copy link
Owner

Choose a reason for hiding this comment

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

We can get venue information using graphql endpoint by requesting event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be awesome if this works! Did you test if we can use this without an API key?

meetup_api.py
import requests
import logging

MEETUP_BASE_URL = "https://www.meetup.com"
Copy link
Owner

@realslimshanky realslimshanky Aug 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The official documentation suggests the use of https://api.meetup.com/gql. Ref: https://www.meetup.com/api/guide/#graphQl-guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this work without an API key?

Copy link
Owner

Choose a reason for hiding this comment

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

Nope. It’s better to use this endpoint.

Copy link
Contributor Author

@ditsuke ditsuke Aug 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this as in the current internal API? I don't think we can get a key for the official API since they restrict it to PRO accounts now.

Copy link
Owner

Choose a reason for hiding this comment

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

@ditsuke We can created an OAuth application and then get the API key once it's approved - https://www.meetup.com/api/oauth/list/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @zora89 mentioned he has already given it a go and been rejected 😞

@engagepy
Copy link
Contributor

engagepy commented Aug 14, 2022 via email

@engagepy
Copy link
Contributor

engagepy commented Aug 15, 2022 via email

@ditsuke
Copy link
Contributor Author

ditsuke commented Aug 15, 2022

Given they don't give access without a Pro subscription, I'm assuming trying again without the funds to back it up is not worthwhile. Also given this circumstance was the precise reason I went in and dug up the internal API that lets us bypass said restriction. I say we should go ahead and get this in for now until Shashank or someone else can get us access to the official and recommended API, at which point it would be rather simple to refactor and take advantage of the additional features the public API allows.

@realslimshanky
Copy link
Owner

@ditsuke Could you please check in PyDelhi group if we have pro meetup.com account, this way we can utilise the API.
It's better to use the official API in that case.

@yednapg
Copy link

yednapg commented Aug 16, 2022

Unfortunately, PyDelhi's meetup.com doesn't have a meetup pro subscription.

@engagepy
Copy link
Contributor

engagepy commented Aug 16, 2022 via email

@realslimshanky
Copy link
Owner

@ditsuke Can you try looking into how we get information about the next meetup on the pydelhi.org page and see if we can use the same API here? Ref: https://github.com/pydelhi/pydelhi.github.io/blob/master/assets/js/main.js

@ditsuke
Copy link
Contributor Author

ditsuke commented Aug 16, 2022

@ditsuke Can you try looking into how we get information about the next meetup on the pydelhi.org page and see if we can use the same API here? Ref: https://github.com/pydelhi/pydelhi.github.io/blob/master/assets/js/main.js

@realslimshanky is this endpoint documented, or in use somewhere by the current meetup.com, or is it a ghost endpoint? If it's the latter case I feel we should stick with the internal endpoint this PR is currently using.

@yednapg
Copy link

yednapg commented Aug 21, 2022

any update?

@realslimshanky
Copy link
Owner

Let's go with this approach for now. It seems like we're not able to use the official API as suggested in the documentation. Also, let's make this version the last one and focus on a separate, more general implementation for a bot, we'll discuss about that later on the call.

@engagepy
Copy link
Contributor

engagepy commented Aug 22, 2022 via email

@realslimshanky realslimshanky merged commit 6781f60 into realslimshanky:master Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@realslimshanky realslimshanky realslimshanky left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ditsuke @realslimshanky @engagepy @progmatic-99 @yednapg