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.

Sign up for GitHub

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

Jump to bottom

weather: Add functions for temperature in Fahrenheit #1785

Closed
wants to merge 1 commit into from
Closed

weather: Add functions for temperature in Fahrenheit #1785

wants to merge 1 commit into from

Conversation

FintasticMan
Copy link
Member

@FintasticMan FintasticMan commented Jun 28, 2023 •
edited
Loading

This adds some functions to the WeatherService class for getting the temperature in Fahrenheit. This could theoretically overflow, but only if the temperature is 164.27 °C or higher (or -164.28 °C or lower), which I believe is unlikely enough that we don't need to worry about it. Please tell me if you think that is an issue.

This doesn't add a setting to display the temperature in Fahrenheit in PTS.

Related to #1783.

@FintasticMan FintasticMan added the enhancement Enhancement to an existing app/feature label Jun 28, 2023
@FintasticMan FintasticMan requested a review from a team June 28, 2023 23:43
@github-actions
Copy link

github-actions bot commented Jun 28, 2023 •
edited
Loading

Build size and comparison to main:

Section Size Difference
text 377496B -16B
data 940B 0B
bss 63420B 0B

@Avamander
Copy link
Collaborator

I really wouldn't add the helper to the data struct definitions, that file is ideally just about the data.

There are other helper functions in the weather controller, that seems like a better place for it?

@FintasticMan
Copy link
Member Author

Ah OK, I was just looking at what you said here, but I've just pushed a change that adds the functions to the WeatherService instead.

Avamander
Avamander approved these changes Jun 29, 2023
View reviewed changes
Copy link
Collaborator

@Avamander Avamander 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 in this form. I'd appreciate if for example @kieranc could test it out with PTS, if he has the time.

@FintasticMan FintasticMan added the weather Bugs and PRs related to Weather label Jul 13, 2023
@FintasticMan FintasticMan mentioned this pull request Jul 15, 2023
Merged
@matoro
Copy link

matoro commented Oct 26, 2023

Hi, wondering if there's anything missing to allow this and the PTS change to be merged? Was very surprised to discover weather service does not support Fahrenheit.

@FintasticMan
Copy link
Member Author

I don't think there's anything blocking this in particular. There are a couple of issues to be worked out with the weather implementation, which take priority over problems like Fahrenheit not being supported.

@FintasticMan
Copy link
Member Author

This is superseded by #1805.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Avamander Avamander Avamander approved these changes

Assignees
No one assigned
Labels
enhancement Enhancement to an existing app/feature weather Bugs and PRs related to Weather
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FintasticMan @Avamander @matoro