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

Nasa commands #363

Closed
wants to merge 6 commits into from
Closed

Nasa commands #363

wants to merge 6 commits into from

Conversation

SurajBhari
Copy link
Contributor

Relevant Issues

Closes #357
Closes #358
Closes #359
Closes #360

Gonna only use pictures from curiosity as only that have some interesting pictures.

Additional Details

Note:-
It needs an API key generated from https://api.nasa.gov/
and stored in env variable named NASA_API

  • Join the Python Discord Community?
  • If dependencies have been added or updated, run pipenv lock?
  • Lint your code (pipenv run lint)?
  • Set the PR to allow edits from contributors?

@SurajBhari SurajBhari requested a review from a team as a code owner February 27, 2020 05:19
Copy link
Member

@ks129 ks129 left a comment

Choose a reason for hiding this comment

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

Please add comments and split code to smaller code blocks.

from bot.constants import Colours, ERROR_REPLIES

log = logging.getLogger(__name__)
api_key = os.environ['NASA_API']
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to constants.py (under Tokens).

api_key = os.environ['NASA_API']


async def fetch(session: aiohttp.ClientSession, url: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved inside of class.

async def fetch(session: aiohttp.ClientSession, url: str) -> str:
"""Helps to get JSON from an API."""
async with session.get(url) as response:
return await response.text()
Copy link
Member

Choose a reason for hiding this comment

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

Use await response.json() to instantly return Dictionary.

class Space(commands.Cog):
"""A cog to get some amazing looking space pictures using NASA API."""

def __init__(self, client: commands.bot):
Copy link
Member

Choose a reason for hiding this comment

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

First, it's commands.Bot and rename client to bot.

@commands.command(description='Astronomy Picture of the Day')
async def apod(self, ctx: commands.context) -> None:
"""Get a cool looking picture from NASA along it's description. That changes every day."""
async with aiohttp.ClientSession() as session:
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use self.bot.http_session instead of making new one

async def mars(self, ctx: commands.context) -> None:
"""Get a random picture from Mars using NASA's API."""
log.info(f'{ctx.author} used mars command.')
async with aiohttp.ClientSession() as session:
Copy link
Member

Choose a reason for hiding this comment

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

Use existing bot http session

log.info(f'{ctx.author} used mars command.')
async with aiohttp.ClientSession() as session:
url = f'https://api.nasa.gov/mars-photos/api/v1/rovers/Curiosity/photos?sol=1000&api_key={api_key}'
response = requests.get(url) # Only Curiosity have Curious pictures.
Copy link
Member

Choose a reason for hiding this comment

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

Don't use blocking requests library.

if int(headers['X-RateLimit-Remaining']) <= 1:
embed = discord.Embed(
title=random.choice(ERROR_REPLIES),
description='API request limit reached for now. Maybe you can try again after some time ?',
Copy link
Member

Choose a reason for hiding this comment

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

Should provide time.

await ctx.send(embed=embed)
return

html = await fetch(session, url)
Copy link
Member

Choose a reason for hiding this comment

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

Use data

return

html = await fetch(session, url)
data = json.loads(html)
Copy link
Member

Choose a reason for hiding this comment

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

No need after L21

@lemonsaurus
Copy link
Member

In the pull request template, we have asked you whether you have remembered to:

  • If dependencies have been added or updated, run pipenv lock?
  • Lint your code (pipenv run lint)?

You said yes to both of these, in spite of the fact that you obviously did not do either of them.

You have also added a dependency, requests, which has no place in an asyncronous bot. The most astounding thing is that you are using it alongside aiohttp in this pull request, which you're using to create a new session instead of using the one that already exists in the bot, like every other cog does.

On top of that, your commits are of poor quality, are not atomic, and have poor commit messages like initial commit and fixing linting - If you had set up the pre-commit like we've asked you countless times to do, you wouldn't need lint commits.

The commit that adds requests contains no rationalization at all. The pull request description is practically nothing even though this adds several features, and the code is poorly documented with no comments and only the bare minimum for docstrings.

When these things were commented on in #dev-contrib, your response was frustration and vitriol.

You requested my review, so here it is: If you're intent on disregarding our advice and making these poor quality pull requests again and again, maybe it's best if you stop contributing to us. I'm closing this PR.

@SurajBhari SurajBhari deleted the nasa-commands branch February 27, 2020 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants