Skip to content
This repository has been archived by the owner on Apr 9, 2021. It is now read-only.

Weather command not parsing location names with spaces #503

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

SuhanKoh
Copy link
Contributor

Fixing issue reported by user, #495

@freyacodes
Copy link
Owner

The regex still won't work for town names containing characters that are not A-Z, for instance:

Tjæreborg
São Paulo
Москва
Clermond-Ferrand
東京

You might also need to trim() the string as well

@freyacodes
Copy link
Owner

You might also want to target the sentinel branch, as this file has been migrated to Kotlin

@SuhanKoh
Copy link
Contributor Author

@Frederikam I feel like migration to kotlin for this change can be on a separate pr. I created this pr intended to fix the issue with spaces in the query

@SuhanKoh
Copy link
Contributor Author

I don't think openweather support searching 東京

@freyacodes
Copy link
Owner

Perhaps not, but query parameters needs to be URL encoded. I don't know if the URL builder handles that

@schnapster
Copy link
Collaborator

Is it just me or does this look like a good case for a unit test?

@freyacodes
Copy link
Owner

The API does not seem to support 東京 or Clermond-Ferrand, but the others work when using curl

@SuhanKoh
Copy link
Contributor Author

SuhanKoh commented Jul 6, 2018

The latest commit supports the other ones except those two. Do you want me to add tests to it? or proceed with integrating with kotlin/sentinel branch?

@freyacodes
Copy link
Owner

If I were you I would migrate to sentinel. I have developed an API to run integration tests on commands that you can use

@freyacodes freyacodes added the stalled-pr Candidate for closure label Feb 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stalled-pr Candidate for closure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants