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

#1742 allowing Choropleth key_on to traverse through array #1772

Merged
merged 7 commits into from
Jul 7, 2023

Conversation

amrutharajashekar
Copy link
Contributor

@amrutharajashekar amrutharajashekar commented Jun 21, 2023

closes #1742

@Conengmo
Copy link
Member

Thanks Amrutha! I'll take a closer look shortly. Just wanted to say already it would be good to add a test for get_by_key. That would require moving it up a level so it becomes a 'proper' method on the class. Since it's not for consumption also make it private by prepending an underscore: _get_by_key.

@amrutharajashekar
Copy link
Contributor Author

Thanks for the pointers Conengmo , will work on them over the weekend and get back to you!

@amrutharajashekar
Copy link
Contributor Author

Hi @Conengmo How do we test for pre-commit check locally ?

@martinfleis
Copy link
Collaborator

@amrutharajashekar

You must first install pre-commit:

pip install pre-commit

From the root of the folium repository, you should then install the pre-commit included in Folium:

pre-commit install

Then checks will be run automatically each time you commit changes. You can skip these checks with git commit --no-verify.

@amrutharajashekar
Copy link
Contributor Author

Hi Conengmo, I have made _get_by_key private and included the test case for the same, Please check this and let me know if any other changes are required. Thanks !

@Conengmo
Copy link
Member

Conengmo commented Jul 1, 2023

Great! I’ll take a look on Monday.

edit: it’ll be later this week, sorry

@Conengmo
Copy link
Member

Conengmo commented Jul 7, 2023

Hi @amrutharajashekar, I added a commit to your PR, hope you don't mind. Your change is good, but since we're touching this code anyway I tried to get rid of some of the repeat logic, make it a bit easier to read. I also added the 'regular' path to the test. If you are okay with this I'll go ahead and merge this one!

@Conengmo Conengmo removed the waiting for review PR is waiting to be reviewed label Jul 7, 2023
@amrutharajashekar
Copy link
Contributor Author

Hi @Conengmo , I don't mind the changes made, we can go ahead and merge them 👍

@Conengmo Conengmo merged commit 8d6bef5 into python-visualization:main Jul 7, 2023
@Conengmo
Copy link
Member

Conengmo commented Jul 7, 2023

@amrutharajashekar thank you very much for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Choropleth key_on doesn't allow for traversing an array
3 participants