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
Contributor

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 python-discord/core-developers as a code owner Feb 29, 2020
@AG4lyf

This comment has been minimized.

Copy link
Contributor

AG4lyf commented Feb 29, 2020

Where are the embed colors ?
they look like washed away

@ks129

This comment has been minimized.

Copy link
Contributor Author

ks129 commented Feb 29, 2020

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

Copy link
Member

SebastiaanZ left a comment

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
ks129 added 3 commits Mar 2, 2020
…, changed embed title, removed HD parameter + changed img from hdurl to url.
…m getting with random.choice
@ks129

This comment has been minimized.

Copy link
Contributor 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 Mar 2, 2020
Copy link
Member

sco1 left a comment

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 Mar 11, 2020
Copy link
Member

sco1 left a comment

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 added 3 commits Mar 14, 2020
@ks129 ks129 requested a review from sco1 Mar 14, 2020
@sco1
sco1 approved these changes Mar 20, 2020
Copy link
Member

ikuyarihS left a comment

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
@ks129 ks129 requested a review from ikuyarihS Mar 31, 2020
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
2 checks passed
2 checks passed
Seasonal Bot Build #20200331.6 succeeded
Details
Seasonal Bot (Lint & Test) Lint & Test succeeded
Details
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
You can’t perform that action at this time.