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

Space Cog implementing #364

Merged
merged 42 commits into from Mar 31, 2020
Merged

Space Cog implementing #364

merged 42 commits into from Mar 31, 2020

Conversation

ks129
Copy link
Member

@ks129 ks129 commented Feb 29, 2020

Relevant Issues

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

Description

I added .apod <date (optional)>, .nasa, .mars <date> and .earth command. Note: Mars command have date limitations.

Reasoning

Due their issues got approved.

Screenshots

Screenshot 2020-02-29 at 17 14 46
Screenshot 2020-02-29 at 17 15 06
Screenshot 2020-02-29 at 17 15 29
Screenshot 2020-02-29 at 17 15 47

Did you:

  • 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?

@ks129 ks129 requested a review from a team as a code owner February 29, 2020 15:16
@SurajBhari
Copy link
Contributor

Where are the embed colors ?
they look like washed away

@ks129
Copy link
Member Author

ks129 commented Feb 29, 2020

@AG4lyf Embed colors is not most important things...

Copy link
Member

@SebastiaanZ SebastiaanZ left a comment

Choose a reason for hiding this comment

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

As a general question, I noticed that we're only using the image urls we get back from the API; do the API responses also include image data? And, if so, do you have an idea about the size of these responses? Are we talking KBs? MB? Multiple MB per request?

bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
@SebastiaanZ SebastiaanZ added level: 0 - beginner status: waiting for author Waiting for author to address a review or respond to a comment type: feature Relating to the functionality of the application. labels Mar 2, 2020
@ks129
Copy link
Member Author

ks129 commented Mar 2, 2020

@SebastiaanZ First, .nasa command always have at least 50 pages (I think they might have thousands pages). And all responses is in KBs. Some commands is just slow due API latency or image size. Images is always URLs, never anything else.

@ks129 ks129 requested a review from SebastiaanZ March 2, 2020 17:34
Copy link
Contributor

@sco1 sco1 left a comment

Choose a reason for hiding this comment

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

As stated in the issues this PR covers, these should all be implemented as subcommands of a single command, rather than as individual commands.

As also stated in the issues this PR covers, there should be some more customization available for the commands to make queries more interesting.

For example:

  • The NASA Image API supports quite a few search options, including a free text search, keyword search, description search, and year start/end constraints
  • The Earth API endpoint supports latitude & longitude queries
  • The Mars command right now only fetches images from the Curiosity Rover and there are 2 other rovers (Opportunity, Spirit) supported by the API

The default being a purely random selection is fine, but please take some time to review the API documentation for each of the endpoints being used here and expand the functionality to provide the capability for some more interesting searches.

As commented on in #350, there needs to be handling for the scenario where the requested page (currently selected randomly) exceeds the number of pages present.

@ks129 ks129 requested a review from sco1 March 11, 2020 15:50
Copy link
Contributor

@sco1 sco1 left a comment

Choose a reason for hiding this comment

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

There are a few instances where multilining doesn't align with the prevailing codestyle in the repository, and also is inconsistent within the cog.

This, for example:

await ctx.send(embed=await self.create_nasa_embed(
    item["data"][0]["title"],
    item["data"][0]["description"],
    item["links"][0]["href"]
))

Should be something like this:

await ctx.send(
    embed=await self.create_nasa_embed(
        item["data"][0]["title"],
        item["data"][0]["description"],
        item["links"][0]["href"]
    )
)

Same with function signatures.

No:

async def fetch_from_nasa(self,
                          endpoint: str,
                          params: Optional[Dict[str, Any]] = None,
                          base: Optional[str] = NASA_BASE_URL
                          ) -> Dict[str, Any]:

Yes:

async def fetch_from_nasa(
    self,
    endpoint: str,
    params: Optional[Dict[str, Any]] = None,
    base: Optional[str] = NASA_BASE_URL
) -> Dict[str, Any]:

bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
@ks129 ks129 requested a review from sco1 March 14, 2020 11:46
@sco1 sco1 added this to Needs review in Seasonalbot Tracking Mar 20, 2020
@sco1 sco1 added priority: 2 - normal status: needs review Author is waiting for someone to review and approve and removed priority: 3 - low status: waiting for author Waiting for author to address a review or respond to a comment labels Mar 20, 2020
Copy link
Member

@ikuyarihS ikuyarihS left a comment

Choose a reason for hiding this comment

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

Every looks pretty good! I only have some small nags, and this should be good to go.

bot/seasons/evergreen/space.py Outdated Show resolved Hide resolved
bot/seasons/evergreen/space.py Show resolved Hide resolved
@ikuyarihS ikuyarihS added status: waiting for author Waiting for author to address a review or respond to a comment and removed status: needs review Author is waiting for someone to review and approve labels Mar 31, 2020
@ks129 ks129 requested a review from ikuyarihS March 31, 2020 05:30
Seasonalbot Tracking automation moved this from Needs review to Reviewer approved Mar 31, 2020
@ikuyarihS ikuyarihS merged commit 4ee977b into python-discord:master Mar 31, 2020
Seasonalbot Tracking automation moved this from Reviewer approved to Done Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Waiting for author to address a review or respond to a comment type: feature Relating to the functionality of the application.
Projects
No open projects
5 participants