pirxpilot / liftie

:ski: Clean, simple, easy to read, fast ski resort lift status.
https://liftie.info
BSD 3-Clause "New" or "Revised" License
67 stars 31 forks source link

A bunch of EU resorts and one japanese resort. #58

Closed PapeSnowiNavi closed 11 months ago

PapeSnowiNavi commented 1 year ago

First attempt at this Pull request stuff, hope that it is right. :)

FezVrasta commented 11 months ago

@pirxpilot ?

pirxpilot commented 11 months ago

Thanks for the PR. Sorry it took me so long to review it.

I am going to add some general remarks here - and also add some comment and suggestions inline later on.

If you can submit a single resort PR we can work out the kinks and land it in liftie. If not I'll try to cherry pick from what I see here. Thanks again.

JeppePape commented 11 months ago

Thanks for the PR. Sorry it took me so long to review it.

I am going to add some general remarks here - and also add some comment and suggestions inline later on.

  • it makes things easier for me if PRs contain one resort or one type of resorts at a time - in this way if there is a problem with one resort it does not block another - a separate branch for each newly added resort and a PR based off that branch makes things much easier; obviously resorts that share the same parser can and should share a PR
  • resort name in the descriptor is something that users see in the UI, resort slug (the name of the directory in which the resort.json resides) is a part of the URL and liftie is always using kebab-case for it - so alta-badia and not Alta-Badia and les-4-valles and not les_4_valles
  • if several resorts have the same parser the common code should end up in /lib/tools - see for example how /lib/tools/vail.js is used in the code
  • ll is not optional and it cannot be set to null - location is used be weather forecast and - where available - webcams
  • please remove the code that you are removing instead of commenting out the old code and adding the new one
  • make sure that the tests are passing before submitting PR

If you can submit a single resort PR we can work out the kinks and land it in liftie. If not I'll try to cherry pick from what I see here. Thanks again.

I see, thanks for the information :)

I was thinking that maybe the whole of the "interwebs" domain could be made into a tool ? under the "tools" folder, similarily to how boyne, aspen and intrawest are. As the way each resort is setup is obv the same and adds a 60-100 ~ areas in especially Austria, but also some other european and international places ?

Then the other ones could be added individually like requested ?

What do you think ?

pirxpilot commented 11 months ago

I was thinking that maybe the whole of the "interwebs" domain could be made into a tool ? under the "tools" folder, similarily to how boyne, aspen and intrawest are. As the way each resort is setup is obv the same and adds a 60-100 ~ areas in especially Austria, but also some other european and international places ?

Then the other ones could be added individually like requested ?

What do you think ?

Yup. That would be my preference as well.

pirxpilot commented 11 months ago

I am pretty sure that all new resorts that were included here (not based on intermaps) are now released. I am going to close this PR now. Please open new/separate PRs once you have more fixes or changes ready. Thanks again!