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

add UIAA gradetype #252

Merged
merged 5 commits into from
Jul 3, 2023
Merged

add UIAA gradetype #252

merged 5 commits into from
Jul 3, 2023

Conversation

l4u532
Copy link
Contributor

@l4u532 l4u532 commented Mar 19, 2023

OpenBeta misses some database fields/types to cover the needs for climbs in Central Europe (Germany, Austria, Switzerland, etc.). For one, the UIAA grading system. For the other, the number of bolts (fixed anchors) for a climb, as sport climbs are the vast majority, trad climbs are more of a niche.

l4u532 added a commit to l4u532/sandbag that referenced this pull request Mar 19, 2023
UIAA is the dominant grading scale in Central Europe. Also see OpenBeta/openbeta-graphql#252.
@l4u532
Copy link
Contributor Author

l4u532 commented Mar 19, 2023

@l4u532 l4u532 changed the title add UIAA gradetype and new datafield bolts add UIAA gradetype and new datafield boltsCount Mar 19, 2023
@l4u532 l4u532 force-pushed the develop branch 2 times, most recently from 2c6f315 to e709c6b Compare March 19, 2023 21:33
@vnugent
Copy link
Contributor

vnugent commented Mar 20, 2023

@l4u532 thanks for the PR. May I ask that you create 2 separate PRs, one for UIAA and one for bolt count field?

For the bolt count, maybe consider using an array to account for multipitch climbs?

@l4u532 l4u532 marked this pull request as draft March 20, 2023 18:51
@bradleyDean
Copy link
Contributor

@l4u532 thanks for the PR. May I ask that you create 2 separate PRs, one for UIAA and one for bolt count field?

For the bolt count, maybe consider using an array to account for multipitch climbs?

How are we dealing with variants of routes? Many multipitch routes have slight variations, where you can break off from the standard route and maybe bypass a difficult section, for example, by climbing a bolt ladder or an easier or harder pitch.
As far as the bolt-count problem goes, how should we handle this?

This really introduces some complexity. Do we already have a method for representing route variants? It seems to me that an array with array index mapping to bolt count per pitch would not easily extend to representing bolt counts for routes with variations. On the other hand, if every variation has its own unique record and name, then there is no problem.
Sorry I am not yet very familiar with the Schema we use for representing multipitch routes.

@musoke musoke mentioned this pull request Apr 19, 2023
@l4u532
Copy link
Contributor Author

l4u532 commented Jun 5, 2023

@musoke @bradleyDean (1) I suggest creating individual routes for when breaking off of the original route (ie. taking a variant). How else would you be able to tick a route.

(2) For "boltsCount", I agree a simple String array should be the most elegant and user-friendly implementation.

As @musoke suggested, grade should then become a String array, to map difficulty to pitch.

@l4u532 l4u532 force-pushed the develop branch 3 times, most recently from 0fcf7f6 to a2835bc Compare June 19, 2023 21:02
@l4u532 l4u532 changed the title add UIAA gradetype and new datafield boltsCount add UIAA gradetype Jun 19, 2023
@l4u532 l4u532 marked this pull request as ready for review June 21, 2023 16:31
@l4u532
Copy link
Contributor Author

l4u532 commented Jun 21, 2023

@musoke @vnugent This adds UIAA grade. Removed boltsCount from this PR (will add in subsequent PR, then start on #266).

Copy link
Contributor

@musoke musoke left a comment

Choose a reason for hiding this comment

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

Could you add some tests? https://github.com/OpenBeta/openbeta-graphql/pull/255/files has analogous ones for Ewbank grades.

@musoke
Copy link
Contributor

musoke commented Jun 21, 2023

Do you want to define a context that uses UIAA too?

UIAA = 'UIAA',
exists but doesn't have its grades defined.

@l4u532
Copy link
Contributor Author

l4u532 commented Jun 21, 2023

Do you want to define a context that uses UIAA too?

UIAA = 'UIAA',

exists but doesn't have its grades defined.

Good spot. Hm, I don't think I want to define it... COUNTRIES_DEFAULT_NON_US_GRADE_CONTEXT expects this, with a correct-looking mapping. Yet, in Central Europe, where UIAA is dominant, people still use a mix of UIAA and French; in my view, one cannot really definitely pinpoint UIAA to a regional context (apart from alpine, maybe). Besides, gradeContext seems to need an overhaul anyway:

export const gradeContextToGradeScales: Partial<Record<GradeContexts, ClimbGradeContextType>> = {

ice, mixed are denominated in WI and MI grades. (Which are missing grades right now).

Do you think it's feasible to leave the definition out for now?

@musoke
Copy link
Contributor

musoke commented Jun 21, 2023

Yet, in Central Europe, where UIAA is dominant, people still use a mix of UIAA and French; in my view, one cannot really definitely pinpoint UIAA to a regional context (apart from alpine, maybe).

Yes, we need more flexibility there.

ice, mixed are denominated in WI and MI grades. (Which are missing grades right now).

I'm working on that, see #283. Likewise for aid.

Do you think it's feasible to leave the definition out for now?

Yes. The main advantage I see is it being an easy way to get some tests in.

@l4u532 l4u532 requested a review from musoke June 28, 2023 11:27
src/graphql/schema/Climb.gql Outdated Show resolved Hide resolved
@musoke
Copy link
Contributor

musoke commented Jun 28, 2023

Looks good! I left one more small query. Do you want to merge this before or after the ice grades in #283? If it's easier for you, I can rebase my PR on this and change the ice grade to WI there.

Could you open an issue describing the problems with the current UIAA context? I think there are similar questions to be addressed with other contexts - e.g. we don't have a way of allowing both winter ice and alpine ice grades, or aid and aid with mandatory free. Addressing these may require reworking the grade context model significantly.

@l4u532
Copy link
Contributor Author

l4u532 commented Jun 28, 2023

Looks good! I left one more small query. Do you want to merge this before or after the ice grades in #283? If it's easier for you, I can rebase my PR on this and change the ice grade to WI there.

I don't mind waiting. Just ping/mention me once you are done :)

Could you open an issue describing the problems with the current UIAA context? I think there are similar questions to be addressed with other contexts - e.g. we don't have a way of allowing both winter ice and alpine ice grades, or aid and aid with mandatory free. Addressing these may require reworking the grade context model significantly.

I must admit I have not fully understood the motivation behind and all the interconnections of the current gradeContext model. It def needs a re-work, though. I'll have a look.

@musoke
Copy link
Contributor

musoke commented Jun 28, 2023

Ok, will ping you when that's done 😄

src/GradeUtils.ts Outdated Show resolved Hide resolved
@l4u532 l4u532 requested a review from musoke July 2, 2023 09:40
@musoke
Copy link
Contributor

musoke commented Jul 3, 2023

@l4u532: sorry, I somehow closed this while trying to rebase your changes on develop. If you either git push -f or merge l4u532#1, we'll be set.
Sorry!

@musoke musoke reopened this Jul 3, 2023
@musoke
Copy link
Contributor

musoke commented Jul 3, 2023

@all-contributors please add @l4u532 for code and ideas

@allcontributors
Copy link
Contributor

@musoke

I've put up a pull request to add @l4u532! 🎉

@musoke musoke merged commit b3e8baf into OpenBeta:develop Jul 3, 2023
2 checks passed
@musoke
Copy link
Contributor

musoke commented Jul 3, 2023

thank you @l4u532!

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.

None yet

4 participants