Skip to content

Conversation

@Ade-StapleHill
Copy link
Contributor

Fix for #1502
First commit of change to the TimeSliderChoropleth test so its no longer an expected fail is
worth taking even if the TimeSliderChoropleth plugin source change is rejected.

@Conengmo Conengmo self-requested a review August 19, 2021 07:00
@Conengmo Conengmo added the ready PR is ready for merging label Nov 9, 2022
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge when all tests pass

Ade-StapleHill and others added 2 commits November 9, 2022 14:56
…ion#1502)

For description of problem see:
 [plugins.TimeSliderChoropleth() time slider bar out of order for date range that spans '2001-09-09' python-visualization#1502](python-visualization#1502)

Files changed:
folium/plugins/time_slider_choropleth.py:
  - try numeric sort first
  - if not numeric e.g. date strings, and exception thrown, then fall back to generic sort.
    - Problem if string as alphabetic sort ('2' > '10') may result in out of order

tests/plugins/test_time_slider_choropleth.py:
  - No longer expected fail
  - Avoid hardwiring in timestamp values
  - Avoid the convenient call to  datetime.strftime('%s') for timestamps as Windows only returns string date
  - Carefully chosen date range to span 2001-09-09

Ran tests and qa:
  - flake8  --max-line-length=120 tests/plugins/test_time_slider_choropleth.py folium/plugins/time_slider_choropleth.py
  - python -m pytest tests/plugins/test_time_slider_choropleth.py (previous commit fixed xfail)
@ocefpaf ocefpaf merged commit c785f43 into python-visualization:main Nov 9, 2022
@ocefpaf
Copy link
Member

ocefpaf commented Nov 9, 2022

Thanks @Ade-StapleHill!

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

Labels

ready PR is ready for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants