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

PR: Add Latitude, Longitude, and Elevation info to the weather station list. #67

Merged
merged 20 commits into from
Oct 4, 2017
Merged

PR: Add Latitude, Longitude, and Elevation info to the weather station list. #67

merged 20 commits into from
Oct 4, 2017

Conversation

jnsebgosselin
Copy link
Owner

@jnsebgosselin jnsebgosselin commented Oct 2, 2017 •
edited
Loading

Fixes #66

New features:

  • Latitude, longitude, and elevation are now stored in the WeatherSationList.
  • It is now obsolete to store the proximity in the station list since we are now able to recalculate it for any geo-coordinate as needed.

GUI improvement:

  • The column that shows the proximity in WeatherStationDisplayTable is now hidden by default. To show the column simply requires to set a reference geo-coordinate in the WeatherStationDisplayTable. The proximity is thus shown in the Search4Stations but is now hidden in DwnldWeatherWidget since it was useless.
  • Sorting was enabled by default in the WeatherStationDisplayTable.

Techdebt:

  • I refactored all the logic to fetch the info from the government of Canada website with BeautifulSoup. This make the code more robust and lighter.
  • We now used WeatherSationList in the StationFinder instead of using a generic list.
  • Make build fails if a test fails in Travis or AppVeyor.

@jnsebgosselin
Refactoring of search_envirocan method
1bb7420 
- Move the try/except logic to open the url to a new method.
- Improved the way the number of station found is evaluated by using the
`re` module capabilities.
@jnsebgosselin
Import the re module
4eda115
@jnsebgosselin
Use BeautifulSoup to read the html
3650e8a
@pep8speaks
Copy link

pep8speaks commented Oct 3, 2017 •
edited
Loading

Hello @jnsebgosselin! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on October 03, 2017 at 18:21 Hours UTC

@jnsebgosselin
Use WeatherSationList in StationFinder instead of a generic list
df2b24d
@jnsebgosselin
Enable sorting in the table
f28eaf6
@jnsebgosselin
Install bs4 on Travis and AppVeyor
9af9d7d
@jnsebgosselin
Fix a bug when adding a new station to WeatherSationList
677e2f4
@jnsebgosselin
Fix a bug relative to adding a new station to WeatherSationList part2
f44a62f
@jnsebgosselin
Added Lat, Lon, and Elev to weather station list
89d7fc0
@jnsebgosselin
Fix mixed str and float type errors
ea139c9
@jnsebgosselin
Move findUnique from StationFinder to the module level
8607dc9
@jnsebgosselin
Make testing more git friendly
f0d9b28
@jnsebgosselin
Remove code duplication
0083863
@jnsebgosselin
pep8
43765dc
@jnsebgosselin
Removed proximity from WeatherStationList
52a621f 
Since we are nw storing latitude and longitude info in the
WeatherStationList, proximity is useless. Since this info is still
usefull in the Search4Station, it is calculated when populating the
WeatherStationDisplayTable. It is also possible to hide the column when
not needed.
@jnsebgosselin
Make build fails if a test fails
9f77557
@jnsebgosselin
Changed digits number of elevation from 3 to 1
b64a573
@jnsebgosselin
Testing
8e94b5b
@jnsebgosselin
Changed the tests to include the changes
093eb13
@jnsebgosselin jnsebgosselin merged commit 58ac18f into master Oct 4, 2017
@jnsebgosselin jnsebgosselin deleted the add_LatLonElev_to_weatherstation_list branch October 4, 2017 01:08
This was referenced Oct 4, 2017
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component: Weather Station Finder type: enhancement type: techdebt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jnsebgosselin @pep8speaks