Skip to content

Conversation

@gravitystorm
Copy link
Contributor

This is the code that @tomhughes wrote, but I've changed upstream so that it generates index.html instead of planet.html. I tested it with test-kitchen and the site generates properly.

tomhughes and others added 2 commits November 28, 2016 22:18
This has changed upstream from planet.html to index.html
@gravitystorm
Copy link
Contributor Author

Could somebody review this PR please?

@tomhughes
Copy link
Member

I don't think there's any problem with the code (well I probably would say that since I wrote most of it ;-)) it's just there wasn't any reason to merge it until we were ready to use it - as it stands it's just an unused cookbook.

The operational question is which machine we're going to run it on...

@gravitystorm
Copy link
Contributor Author

it's just there wasn't any reason to merge it until we were ready to use it

Fair enough. I wasn't sure from the silence whether there were problems or not or if you we reconsidering anything.

@zerebubuth
Copy link
Contributor

I think there's are good reasons to merge it now, even if it's currently unused:

  1. Being merged into master instead of on a branch means the code won't accumulate merge conflicts that we have to fix later. I know the conflicts will exist anyway, and having it on a branch simply delays that work. However, in my experience it often builds up to levels which are onerous, and end up discouraging further progress on the PR.
  2. Having lots of open PRs, especially stale ones, splits attention and conversation over many concurrent comment streams. I think it is also widely assumed that it indicates an unmaintained project.

We plan to use this, so it's not unnecessary code - just unused at present. In my opinion, the best option is to merge it and deal with any further changes in a future PR, if any are necessary when we launch it.

@tomhughes tomhughes merged commit 4f37c04 into openstreetmap:master Jan 26, 2017
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