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

Created a CLI for the package, made 'area_keys' and 'polygon_features' variable #32

Merged
merged 9 commits into from
Mar 22, 2022

Conversation

CrsiX
Copy link
Contributor

@CrsiX CrsiX commented Mar 17, 2022

Referencing #25

This PR adds a command-line interface to the osm2geojson package. It should be executed as python3 -m osm2geojson.
Additionally, it allows the area_keys and polygon_features to be set as optional arguments (they are just carried around with in most of the functions).

I've marked this is a draft since I first wanted to get some feedback and suggestions for improvement from you, @rapkin :)

Copy link
Collaborator

@rapkin rapkin left a comment

Choose a reason for hiding this comment

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

Thank you for you work! It's very important for this package to have good cli-tool.
We are very close to point when this PR can be merged, but we need several improvements.
You can find my recommendations in comments.
Also, to be confident about our work, we need to add tests for cli tool as well (parsing of agruments). What you think about this?

if indent and indent < 0:
indent = None
if args.outfile == "-":
target = sys.stderr
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that we need stderr here (it's for errors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, my intention was to use stdout for this (as documented in setup_parser) :)


Other handy methods:

- `overpass_call(str query)` - runs query to overpass-api.de server (retries 5 times in case of error).
- `shape_to_feature(Shape shape, dict properties)` - Converts Shape-object to GeoJSON with passed properties.

**\*Shape-object - for convinience created simple dict to save Shapely object (geometry) and OSM-properties. Structure of this object:**
**\*Shape-object - for convenience created simple dict to save Shapely object (geometry) and OSM-properties. Structure of this object:**
Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you for this)

Just call Python with the `-m` option and the package name:

```sh
python3 -m osm2geojson --help
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be possible to use this cli tool without python3 -m.
I found good article about this here https://pybit.es/articles/how-to-package-and-deploy-cli-apps/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! I've used the entry_points feature in the setup.py for this

)
parser.add_argument(
"--format",
choices=("geo", "shape"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that I understand how 'shape' format should behave. You may find that, for example xml2shapes produces Shapely objects (it's complex in-memory structure, example <shapely.geometry.point.Point object at 0x...>) and we can't stringify these objects to json directly. We need such output to prevent redundant transformation between formats (just for optimization), so that's why we have additional methods (like xml2shapes). And you can use this output only in a programmatic way. I think we can make cli tool without 'shape' fromat.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we don't need '--format' option at all. Only one output format - geojson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't need '--format' option at all. Only one output format - geojson

Okay, I will do it that way then.

@CrsiX
Copy link
Contributor Author

CrsiX commented Mar 20, 2022

Also, to be confident about our work, we need to add tests for cli tool as well (parsing of agruments). What you think about this?

Testing the parsing of arguments is out of scope for this. I would refer to argparse and the wide use of that package. I agree that test coverage is really great, but that should stop at something like CLI argument parsing. After all, the use of the CLI is optional, since it only provides a wrapper around the functionality provided by the library. Therefore, the library should have good test coverage, but not the CLI.

I would instead suggest to add a new entry in the GitHub Actions Workflow that builds the pip package and then installs & runs it locally once (just to be sure it works as intended).

@CrsiX
Copy link
Contributor Author

CrsiX commented Mar 21, 2022

I would suggest to add something like this to the end of .github/workflows/pythonpackagetest.yml:

- name: Build the package
  run: |
    pip install setuptools wheel
    python setup.py sdist bdist_wheel
- name: Install the local package and execute it
  run: |
    pip install ./dist/osm2geojson-*.tar.gz
    python -m osm2geojson --help
    osm2geojson --help

It ensures that the pip package can be installed successfully and that the module as well as the script osm2geojson can be executed. Additionally, one may add the following to ensure the same inside a virtual environment:

- name: Install the local package in a venv and execute it
  run: |
    sudo apt install python3-venv -y
    python3 -m venv venv
    . venv/bin/activate
    pip install ./dist/osm2geojson-*.tar.gz
    test -f venv/bin/osm2geojson
    python -m osm2geojson --help
    osm2geojson --help

@rapkin
Copy link
Collaborator

rapkin commented Mar 21, 2022

I agree, we can add new step to already existing action (to test installation of cli tool). But I think that It's good to have some tests over cli tool. Tests are showing how to use our package + additional layer of validation. I can write those tests after merge, no problem.
Your work is awesome, thank you!

@rapkin
Copy link
Collaborator

rapkin commented Mar 22, 2022

I'm trying to install package on my computer

> python setup.py sdist bdist_wheel
> pip install ./dist/osm2geojson-*.tar.gz
...
ERROR: For req: osm2geojson==0.1.32. Invalid script entry point: <ExportEntry osm2geojson = osm2geojson.__main__.main:None []> - A callable suffix is required. Cf https://packaging.python.org/specifications/entry-points/#use-for-scripts for more information.

I changed the code a bit to 'console_scripts': ['osm2geojson=osm2geojson.__main__.main:main'] and it works.

Maybe you can add this small change and I'll approve your PR. Thank you again!

@CrsiX
Copy link
Contributor Author

CrsiX commented Mar 22, 2022

Thank you :)

@rapkin rapkin marked this pull request as ready for review March 22, 2022 01:05
@rapkin rapkin merged commit f95b346 into aspectumapp:master Mar 22, 2022
@CrsiX CrsiX deleted the cli branch March 22, 2022 01:06
@rapkin rapkin mentioned this pull request Mar 22, 2022
@CrsiX CrsiX changed the title WIP: Created a CLI for the package, made 'area_keys' and 'polygon_features' variable Created a CLI for the package, made 'area_keys' and 'polygon_features' variable Apr 5, 2022
@epsawan
Copy link

epsawan commented Sep 15, 2023

I want to get separate object of point, linestrime and polygon. How can I do that

@epsawan
Copy link

epsawan commented Sep 15, 2023

I want tags out like in general format :
tags : {
name:...
other tags....
}

but I want like this: (don;t want tags object)
name:....
other tags...

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.

3 participants