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

Fix TypeError for number values in settings #2258

Merged
merged 1 commit into from Aug 17, 2019

Conversation

@krisgesling
Copy link
Contributor

commented Aug 16, 2019

In some instances, the backend returns number setting values as an integer rather than a string.

This seems to be the quickest fix for it, but worth investigating further from the backend.

How to test

This has been seen in the wild but has been difficult to replicate. The circumstances under which it happens are still unclear. I was able to semi-consistently have a number returned as int from the Severe Weather Skill when editing an unrelated setting. To test, remove the 3 character country code ([A-Z]{3} - ) from the service options of this skill .

Contributor license agreement signed?

In some instances, the backend returns number setting values as an integer rather than a string.

This has been seen in the wild but has been difficult to replicate. The circumstances under which it happens are still unclear. I was able to semi-consistently have a number returned as int from the [Severe Weather Skill](https://github.com/domcross/severe-weather-information-skill) when editing an unrelated setting. To test, remove the 3 character country code (`[A-Z]{3} - `) from the service options of this skill .

This seems to be the quickest fix for it, but worth investigating further from the backend.
@krisgesling

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@chrisveilleux - just FYI - I ran into the same TypeError reported by malevolent in the chat today

@chrisveilleux

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

@chrisveilleux - just FYI - I ran into the same TypeError reported by malevolent in the chat today

Yeah, I am of the opinion that the logic in core needs to be changed. I am working on a change right now that plays more nicely with the backend. This will do as a temp fix until I can get my new thing working.

@forslund

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

@chrisveilleux Just remember that the backend needs to be backwards compatible and should send these numbers as strings so older versions of core will still work.

@forslund forslund merged commit dd36678 into dev Aug 17, 2019
3 of 4 checks passed
3 of 4 checks passed
License Compliance 11 issues found
Details
:-) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on fix/number-setting-values at 51.723%
Details
@forslund forslund deleted the fix/number-setting-values branch Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.