Fixed open_browser issue and added functionality for map and config file#6
Fixed open_browser issue and added functionality for map and config file#6ikespand wants to merge 12 commits intokylebarron:masterfrom
Conversation
ikespand
commented
Jul 17, 2020
- Fixed issue-2 regarding the 'open_browser=False still opens browser '
- added functionality to save maps at custom location
- added functionality to use a custom json config file
- added corresponding changes to the README.md
…fault config JSON
|
Hey @kylebarron, do you plan to merge this pull request? The addition of |
|
This PR is stale and has merge conflicts/needs to be updated with master. but in principle not opposed to merging |
|
Ok. Do you plan to add those two arguments in the near future? That would be really helpful! |
|
I don't have plans to work on this repo myself, but PRs are welcome. It probably wouldn't be too hard to update this PR with changes in |
|
Hey guys. I will resolve the conflicts by today and will commit again. |
…SON and provide open_browser in init
|
@kylebarron : Resolved and revised. If you've any feedbacks then let me know. And if you decide to merge then you might need to change date in |
|
Thanks @ikespand! Hope @kylebarron merges these changes soon. :) |
kylebarron
left a comment
There was a problem hiding this comment.
Generally a good approach but some comments. Also, any new parameters to the Python API should also be reflected in the CLI as well
| `config_file` provides the path of config file. | ||
| `output_map` provides the location html file, if none then will | ||
| be dumped to temporaty files. | ||
| `open_browser` enables the browser opening if data is provided. |
There was a problem hiding this comment.
These params should be in the same format as data above
| api_key=None, | ||
| style=None): | ||
| style=None, | ||
| config_file=None, |
There was a problem hiding this comment.
IMO this should accept a config as a python dict, not as a path to a file
| style=None, | ||
| config_file=None, | ||
| output_map=None, | ||
| open_browser=False): |
There was a problem hiding this comment.
I originally didn't intend open_browser to be exposed through __init__. The rationale being that passing data into here is just a shortcut for using render directly, and we don't need to expose all the same params
|
|
||
| ```py | ||
| Visualize(data=None, names=None, read_only=False, api_key=None, style=None) | ||
| Visualize(data=None, names=None, read_only=False, api_key=None, style=None, config_file=None, output_map=None) |
There was a problem hiding this comment.
See below for discussion of whether open_browser should exist as a param to __init__, but if it does exist then it should be documented here
| else: | ||
| self.config_file = config_file | ||
| if output_map is not None: | ||
| self.path = output_map+'_vis.html' |
There was a problem hiding this comment.
If the user passes in a specific path, we shouldn't change it
| tests_require=test_requirements, | ||
| url='https://github.com/kylebarron/keplergl_cli', | ||
| version='0.3.3', | ||
| version='0.3.4', |
There was a problem hiding this comment.
You shouldn't change the version here; we'll have a separate PR to bump the version, which will probably be 0.4.0
|
|
||
| return path | ||
| webbrowser.open_new_tab('file://' + self.path) | ||
| return self.path No newline at end of file |
There was a problem hiding this comment.
Nit: here and below are missing a newline at the end of the file
|
|
||
| def render(self, open_browser=True, read_only=False, center_map=True): | ||
| """Export kepler.gl map to HTML file and open in Chrome | ||
| """Export kepler.gl map to HTML file and open in defauly system browser |
There was a problem hiding this comment.
| """Export kepler.gl map to HTML file and open in defauly system browser | |
| """Export kepler.gl map to HTML file and open in default system browser |
| # First load config file as string, replace {MAPBOX_API_KEY} with the | ||
| # actual api key, then parse as JSON | ||
| with open(config_file) as f: | ||
| with open(self.config_file) as f: |
There was a problem hiding this comment.
This function shouldn't touch a user-provided config file.
| read_only=False, | ||
| api_key=None, | ||
| style=None): | ||
| style=None, |
There was a problem hiding this comment.
8 params is too many to allow to be positional imo. I'd rather not change the existing API but we should add * before config_file to make the new params keyword-only
|
@ikespand, did you get a chance to work on this? |
|
Sorry @ashirwad .. Things were little chaotic here. But, will commit the suggestions within 2 weeks. |
|
No worries, @ikespand! Hope you are doing well. And thanks again for all your help with this. :) |