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

Create weather.py #428

Merged
merged 10 commits into from May 7, 2019

Conversation

Projects
None yet
3 participants
@bradleysigma
Copy link
Contributor

commented Apr 30, 2019

No description provided.

bradleysigma added some commits Apr 30, 2019

Update weather.py
Apparently python 3.7 allows colons in `%z`
@CameronAavik

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Do you have a screenshot of the functionality? Or details on what this PR does?

bradleysigma added some commits May 1, 2019

Update test_weather.py
Original script only worked on creation date
Update weather.py
Split into functions, added daily schedule.
@bradleysigma

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

image
Will also post today's Brisbane weather at 6:00am.
Emojis don't exit on uqcstesting server, but do on UQ Computing Society.
Reimplements weather.js in unimplemented_commands.

@nicklambourne
Copy link
Member

left a comment

This looks really good but could really use some fstrings throughout + addressing some other nits.

Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
# get responses
response = []
brisbane_detailed, brisbane_fire, brisbane_uv = response_brisbane_detailed()
response.append(response_header(node, location))

This comment has been minimized.

Copy link
@nicklambourne

nicklambourne May 3, 2019

Member

Could all of this be in a single fstring? If not it could certainly use a helper method.

This comment has been minimized.

Copy link
@bradleysigma

bradleysigma May 3, 2019

Author Contributor

Possibly, but it would likely look inelegant, especially as some fields don't exist depending on time of day and days into the future the forecast is for etc.

Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py Outdated
Show resolved Hide resolved uqcsbot/scripts/weather.py

bradleysigma added some commits May 3, 2019

@bradleysigma bradleysigma requested a review from nicklambourne May 3, 2019

@nicklambourne
Copy link
Member

left a comment

lgtm, those fstrings and types make a real readability difference.



def get_xml(state: str):
def get_xml(state: str) -> Union[None, ET.Element]:

This comment has been minimized.

Copy link
@nicklambourne

nicklambourne May 7, 2019

Member

Could be ... -> Optional[ET.Element]

@nicklambourne nicklambourne merged commit 8f0aa0f into master May 7, 2019

1 check passed

Jenkins 42 tests run, 0 skipped, 0 failed.
Details

@bradleysigma bradleysigma deleted the weather branch May 8, 2019

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.