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

Update docs #3881

Merged
merged 8 commits into from
Jan 2, 2023
Merged

Update docs #3881

merged 8 commits into from
Jan 2, 2023

Conversation

nilsnolde
Copy link
Member

fixes #3797

Let me know if you find anything is too aggressive here. IMO it looks quite a bit less intimidating to newcomers but still preserves all info that was there previously.

…st instructions; add section about related projects
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

Nice updates! I noticed a couple tiny things.

docs/releasing.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@nilsnolde
Copy link
Member Author

Thanks @michaelkirk for spotting the typos!

@kevinkreiser this is ready from my side.

@ImreSamu
Copy link
Contributor

ImreSamu commented Dec 30, 2022

✔️

Thank you for updating. 😄

Suggestions for the README:

Adding FUNDING.yml (A Github Sponsorship button ) might also be an idea to consider. ( collecting microdonations for the Valhalla project )

EDIT:
for a Data license ; we can link the https://github.com/valhalla/valhalla/blob/master/docs/mjolnir/data_sources.md

@nilsnolde
Copy link
Member Author

IMO the LICENSE.md is only applicable for the code of this repo (if that’s what you mean).

GitHub sponsors we discussed (and sorta dismissed) before.

Regarding mentioning OSMF, I’d be fine with that, don’t have a strong opinion there. What do the others think?

@ImreSamu
Copy link
Contributor

IMO the LICENSE.md is only applicable for the code of this repo (if that’s what you mean).

In my opinion, the README should also refer to the licensing requirements of the default OpenStreetMap data of the running Valhalla system. ( And linking the data_sources.md )

This may be important information for a new user, especially if they are not familiar with the licensing of OpenStreetMap data.
And it is my experience that very few people are aware of the licensing of OpenStreetMap data, so a warning in the README is a good idea.

If we want a “list of ingredients” of the running Valhalla system, the mentioning the full data_sources.md and licensing is also important!

disclaimer: I am an OSM mapper and member of the OSMF and I don't want to see valhalla-related problems in this list https://wiki.openstreetmap.org/wiki/Lacking_proper_attribution 😄

@ImreSamu
Copy link
Contributor

ImreSamu commented Dec 30, 2022

II.

IMO the LICENSE.md is only applicable for the code of this repo (if that’s what you mean).

ouch ...

IMHO: the osm test data in this repo ( ls ./test/data/*.osm.pbf ) is probably also ODBL license.
( ~ derived from real osm data ?? )
(see https://github.com/valhalla/valhalla/tree/master/test/data )

So we need to mention in the LICENSE.md + adding a proper attribution

@kevinkreiser
Copy link
Member

@ImreSamu its an interesting point but seems rather esoteric when scoped to some tiny extracts used only for unit testing. it would be great to just delete it all and call it covered by gurka (or add a few tests) but it is less work to just add proper attribution so we'll probalby have to do that in the short term. good catch but also, not a big deal imho

@ImreSamu
Copy link
Contributor

@kevinkreiser :

good catch but also, not a big deal imho

I don't think it's a serious problem either. And my comment is more of a surprise, as I'm usually sensitive to OSM license stuff and I hadn't noticed this. 🥲

In case the osm.pbf test data is moved to another test repo ( or new test data will be added )
it is worth removing the OSM metadata ( especially the OSM contributor uid= and user= metadata ) and then the EU GDPR regulations are also satisfied. ( --> https://wiki.openstreetmap.org/wiki/GDPR )

Sorry .. I don't want to block this PR, and perhaps my concerns about the OSM license should be addressed in another PR.

@nilsnolde
Copy link
Member Author

updated with info on data licenses in 6699e9e

@ImreSamu
Copy link
Contributor

ImreSamu commented Jan 1, 2023

@nilsnolde

updated with info on data licenses

✔️ Thank you! All my comments have been addressed. 💯

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
1. Take a giant file with routes with > 10k
2. Spin up the service
3. Run the requests with `./run_route_scripts/run_with_server.py`: we can alter the logic for the requests inside that script, depending what we want to test
4. E.g. `./run_route_scripts/run_with_server.py --test-file auto.txt --url http://localhost:8002/route --concurrency 20 --format csv`
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -314,3 +117,16 @@ It's important to note that all Valhalla logs for one-shot mode are piped to `st
### Batch Script Tool

- [Batch Run_Route](./run_route_scripts/README.md)

## Related projects
Copy link
Member

Choose a reason for hiding this comment

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

i like this. its been missing for a long time. i cant help but thing there is more we should put here but at the moment i just cant put my finger on what is missing.. anyway thanks for starting the list!

CONTRIBUTING.md Outdated
4. E.g. `./run_route_scripts/run_with_server.py --test-file auto.txt --url http://localhost:8002/route --concurrency 20 --format csv`


## Tranlation contributions

This comment was marked as resolved.

@nilsnolde nilsnolde requested review from kevinkreiser and removed request for michaelkirk January 2, 2023 04:28
@nilsnolde nilsnolde merged commit 81fe0af into master Jan 2, 2023
@nilsnolde nilsnolde deleted the nn-update-docs branch January 2, 2023 13:14
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.

Update README with more (and less) info
4 participants