Closed enapupe closed 1 year ago
Statements coverage not met for global: expected <=999999 not covered statements, but got 608 | St.:grey_question: |
Category | Percentage | Covered / Total |
---|---|---|---|---|
π’ | Statements | 87.48% (+1.09% πΌ) |
608/695 | |
π‘ | Branches | 67.54% (+2.22% πΌ) |
129/191 | |
π‘ | Functions | 79.87% (+0.45% πΌ) |
119/149 | |
π’ | Lines | 86.67% (+1.1% πΌ) |
559/645 |
268 tests passing in 17 suites.
Report generated by π§ͺjest coverage report action from 05900e25a9fd3ff712407a6e229ab83898547efd
I suggest we start using a code formatter like prettier, life is too short to manually adjust code looks.
I am also a fan of code formatters. Most of OpenBeta's repos already use ts-standard
- please open a separate issue or PR if you think that isn't enough.
I suggest we start using a code formatter like prettier, life is too short to manually adjust code looks.
I am also a fan of code formatters. Most of OpenBeta's repos already use
ts-standard
- please open a separate issue or PR if you think that isn't enough.
To be honest I haven't paid much attention to it, I remember I quickly runned it with --fix
and didn't noticed it also formatted the code besides checking for types.
I guess one code formatter is sufficient :)
As far as I can tell you're using it to avoid duplicating array definitions in src/scales/brazilian.ts and src/index.ts. Is that correct?
Yes, exactly. I was trying to keep things consistent while not duplicating that array. The exposed API in somewhat unknown to me so that's why I did that change to access the array. Another option that crossed my mind was to actually generate that array from the json file, just getting the grades (ordered) and making it a set (i.e. removing dupes). Then it's automatically generated from within.
Wikipedia says that there are at least two scales used in Brazil. The one you have implemented is what they call the "Brazilian technical scale". Should the name in sandbag be something like BrazilianTechnical? This will reduce confusion if we add additional Brazilian scales later.
This information may be outdated, currently there's a single scale used in Brazil. In 2007 the brazilian National Confederation of Mountaineering and Climbing has published this spec and incentivized its use as the only standard
@musoke thank you for the throughout review! Really appreciate it.
How familiar are you with Brazillian grades? I am not, so I will defer to you on much of this if you're a local.
I'm brazilian and have been climbing around the country the last 10 years, the brazilian spec is not really that complex, but there are many misleading informations out there, especially from english sources. I think that's because in the past there was no official source of truth, but that was way before I started climbing so I can't tell for sure.
The spec is pretty official and well established across the country.
On Thu, Nov 9, 2023 at 4:03β―PM nathan musoke @.***> wrote:
@.**** commented on this pull request.
In src/data/routes.csv https://github.com/OpenBeta/sandbag/pull/158#discussion_r1388466331:
+8,5.0,1c,1,2,1,1+,Isup +9,5.0,1c,1,2,1,1+,Isup +10,5.0,1c+,1,3,1,2-,Isup +11,5,1c+,1,4,1,2-,Isup +12,5.1,2a,2,5,2,2,Isup +13,5.1,2a,2,5,2,2,Isup
the spec https://www.cap.com.br/post/sistema-brasileiro-de-gradua%C3%A7%C3%A3o-de-vias-de-escalada
Nice, searching in English wasn't sufficient to find that.
How official is this spec? Maybe there should be a link to it in the comments. Then your reasoning will be clearer to anyone who changes this code in future.
β Reply to this email directly, view it on GitHub https://github.com/OpenBeta/sandbag/pull/158#discussion_r1388466331, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACHCCVUK5A5ASKHSJE7Y6DYDUSJPAVCNFSM6AAAAAA7BGKUJWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRTGMYDGNJSGE . You are receiving this because you authored the thread.Message ID: @.***>
Wikipedia says that there are at least two scales used in Brazil. The one you have implemented is what they call the "Brazilian technical scale". Should the name in sandbag be something like BrazilianTechnical? This will reduce confusion if we add additional Brazilian scales later.
This information may be outdated, currently there's a single scale used in Brazil. In 2007 the brazilian National Confederation of Mountaineering and Climbing has published this spec and incentivized its use as the only standard
Searching in English wasn't sufficient to find that very clear and thorough spec :laughing:
Thank you for the links and detailed explanation!
Could you add a link to the spec in a docstring for const Brazilian
? Then the intended behaviour will be clearer to anyone who uses this or changes it in the future.
Does anyone use grades that differ from the spec in ways that matter? For example, in old guidebooks? Are there routes whose consensus grade is non-compliant? I think we should lean descriptivist if real-world grades diverge from the spec.
As far as I can tell you're using it to avoid duplicating array definitions in src/scales/brazilian.ts and src/index.ts. Is that correct?
Yes, exactly. I was trying to keep things consistent while not duplicating that array. The exposed API in somewhat unknown to me so that's why I did that change to access the array. Another option that crossed my mind was to actually generate that array from the json file, just getting the grades (ordered) and making it a set (i.e. removing dupes). Then it's automatically generated from within.
Yes, that part of the exposed API is mysterious to me too. As far as I can tell, it is leftover from early versions. Let's keep what you wrote and address the other issues later.
Does anyone use grades that differ from the spec in ways that matter? For example, in old guidebooks? Are there routes whose consensus grade is non-compliant?
People often mix arabic/romans for single pitch sports route, which is wrong and should not be encouraged if we want to keep things consistent/within the spec. Aside from that, I don't have any relevant information/knowledge. I'd have to research old magazines maybe, but I'm not sure if that's relevant.
Does anyone use grades that differ from the spec in ways that matter? For example, in old guidebooks? Are there routes whose consensus grade is non-compliant?
People often mix arabic/romans for single pitch sports route, which is wrong and should not be encouraged if we want to keep things consistent/within the spec. Aside from that, I don't have any relevant information/knowledge. I'd have to research old magazines maybe, but I'm not sure if that's relevant.
Arabic -> Roman is a straightforward mapping, so likely no information loss there. Let's not worry about it if the edge cases are obscure.
@all-contributors add @enapupe for code and ideas
@musoke
I've put up a pull request to add @enapupe! :tada:
hey guys, I think this is ready to be reviewed. My changes were based on the norwegian scale.
I did a minor "API" change in the
GradeScale
types to accomodate the actual list of grades, that was necessary to handle cases where a slash grade was not the next logical grade.I suggest we start using a code formatter like prettier, life is too short to manually adjust code looks.