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

Imgur: Allow use with an account and album #402

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

half-duplex
Copy link
Contributor

I'd like to have the ability to collect and manage uploaded images. The easiest way to do this is to upload to an Imgur account instead of anonymously, and to an album.

As I said in the other PR, I don't really speak golang, so I appreciate comments.

Note: Imgur's documentation states that access tokens are valid for 1 month, but the ones the API returns seem to not expire for 10 years. If they ever change this, bots in this configuration running for over 1 month will need a restart to refresh the token.

@jwflory jwflory added new change Adds new capabilities or functionality needs review Pending a peer review before merging or closing Telegram Issues relating to Telegram bridge labels Jun 11, 2022
@jwflory jwflory requested a review from a team June 11, 2022 04:22
@jwflory
Copy link
Member

jwflory commented Jun 11, 2022

Thanks for the extensive documentation on this Pull Request @half-duplex. It's especially appreciated.

I just changed a setting for the CircleCI pipeline where it should properly build Pull Requests submitted from a forked repository. If you push a new commit to this Pull Request, the CI builds should execute completely.

@half-duplex half-duplex force-pushed the imgur-account branch 3 times, most recently from 2149920 to f44b8e9 Compare June 11, 2022 05:49
@jwflory
Copy link
Member

jwflory commented Jun 14, 2022

All tests passed except the Go 1.17 tests. This is because this test suite also uses a secret credential to upload the test coverage results to CodeClimate. The secret credential is only available on branches on this repository.

So, all test suites are passing here. There is some feedback from CodeClimate about too many return statements in the getImgurAccessToken function, but I'm not sure this is something we have to mitigate now.

Copy link
Member

@Tjzabel Tjzabel left a comment

Choose a reason for hiding this comment

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

Overall looks really good! Just had a few comments and spelling error.

Also not sure if there's a better way to format the returns in imgur.go. For me I'm fine with the number of returns.

docs/user/faq.rst Outdated Show resolved Hide resolved
docs/user/faq.rst Outdated Show resolved Hide resolved
@jwflory jwflory removed the needs review Pending a peer review before merging or closing label Jun 21, 2022
Copy link
Member

@jwflory jwflory left a comment

Choose a reason for hiding this comment

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

LGTM. @Tjzabel or any @RITlug/teleirc-developers-commit-access member, want to take a final pass here?

@jwflory
Copy link
Member

jwflory commented Jun 22, 2022

I rebased this PR on the latest changes in main via the GitHub web interface.

Copy link
Member

@Tjzabel Tjzabel left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making these changes 👍

@Tjzabel Tjzabel merged commit 8e98e22 into RITlug:main Jun 22, 2022
@half-duplex half-duplex deleted the imgur-account branch July 10, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new change Adds new capabilities or functionality Telegram Issues relating to Telegram bridge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants