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

Use getattr instead of eval #207

Merged
merged 2 commits into from Feb 25, 2022
Merged

Use getattr instead of eval #207

merged 2 commits into from Feb 25, 2022

Conversation

Piphi5
Copy link
Contributor

@Piphi5 Piphi5 commented Feb 25, 2022

getattr is more robust and cleaner than trying to use eval for getting parameters

@giswqs
Copy link
Member

giswqs commented Feb 25, 2022

It seems get_attr does not exist. Should it be getattr?

@Piphi5
Copy link
Contributor Author

Piphi5 commented Feb 25, 2022

Yeah it is, my bad.

@Piphi5 Piphi5 changed the title Use get_attr instead of eval Use getattr instead of eval Feb 25, 2022
@giswqs
Copy link
Member

giswqs commented Feb 25, 2022

It is still failing. I am not sure if they are caused by the tests only or getattr changes result in failed tests. Can you fix it?

@Piphi5
Copy link
Contributor Author

Piphi5 commented Feb 25, 2022

Yeah I'll look into it. It seems like trying to call "OpenStreetMap.Mapnik" as a single parameter may be the main issue

@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2022

This pull request fixes 3 alerts when merging 98d74ee into c28a48a - view on LGTM.com

fixed alerts:

  • 3 for Suspicious unused loop iteration variable

@giswqs
Copy link
Member

giswqs commented Feb 25, 2022

It is improving. Only test_add_circle_markers_from_xy is failing.

@Piphi5
Copy link
Contributor Author

Piphi5 commented Feb 25, 2022

Is there any reason why this:

out_str = m.to_html()
assert "World Cities" in out_str

would fail with add_circle_markers_from_xy but not fail with add_xy_data?

@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2022

This pull request fixes 3 alerts when merging 1b051a8 into d5d4248 - view on LGTM.com

fixed alerts:

  • 3 for Suspicious unused loop iteration variable

@giswqs
Copy link
Member

giswqs commented Feb 25, 2022

It should be name rather than layer_name. It seems add_circle_markers_from_xy is not producing the expected results. There is only one circle plotted on the map. It seems there is a bug with this function, but it might not be related to your commits.

import leafmap
import ipyleaflet

m = leafmap.Map()
in_csv = "https://raw.githubusercontent.com/giswqs/data/main/world/world_cities.csv"

m.add_circle_markers_from_xy(
    in_csv, x="longitude", y="latitude", name="World Cities"
)
m.add_control(ipyleaflet.LayersControl(position="topright"))
m

image

@Piphi5
Copy link
Contributor Author

Piphi5 commented Feb 25, 2022

is the add_circle bug reproducable without the code in this pr?

@giswqs
Copy link
Member

giswqs commented Feb 25, 2022

It is a bug of the ipyleaflet backend. Works fine with folium.

@giswqs giswqs merged commit 84ad70c into opengeos:master Feb 25, 2022
@Piphi5 Piphi5 deleted the use_get_attr branch February 25, 2022 22:17
@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2022

This pull request fixes 3 alerts when merging 05b6daa into d5d4248 - view on LGTM.com

fixed alerts:

  • 3 for Suspicious unused loop iteration variable

giswqs added a commit that referenced this pull request Feb 28, 2022
sthagen pushed a commit to sthagen/giswqs-leafmap that referenced this pull request Feb 18, 2023
Use getattr instead of eval

Former-commit-id: 84ad70c
sthagen pushed a commit to sthagen/giswqs-leafmap that referenced this pull request Feb 18, 2023
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.

None yet

2 participants