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

slash grades should have integer scores #138

Merged

Conversation

johncalvinroberts
Copy link
Contributor

Possible tentative fix for: #137

@musoke
Copy link
Collaborator

musoke commented Jul 28, 2023

I suggest two changes here:

  • round the top and bottom of the range in opposite directions
  • add some tests

@johncalvinroberts johncalvinroberts marked this pull request as ready for review July 30, 2023 20:16
@johncalvinroberts
Copy link
Contributor Author

@musoke updated according to your suggested implementation. Thank you so much for the guidance!

After updating, some tests (integration tests?) in readme.ts were failing due to hardcoded non-integer return values. This caused me to notice that French scale implementation was also suffering from the same defect, and could potentially return non-integer score values for slash grades. So, I updated with the same logic, assuming slash grades to be exclusive of adjacent grades in the french scale. Is that a correct?

Also, definitely LMK if there are any other issues with my implementation and I'll try to fix.

@musoke
Copy link
Collaborator

musoke commented Jul 31, 2023

After updating, some tests (integration tests?) in readme.ts were failing due to hardcoded non-integer return values. This caused me to notice that French scale implementation was also suffering from the same defect, and could potentially return non-integer score values for slash grades. So, I updated with the same logic, assuming slash grades to be exclusive of adjacent grades in the french scale. Is that a correct?

Yes, now that you point it out, this this is going to be an issue with all the scales. Maybe you could define a function to do this check for each scales. Or I can add it to other scales in a followup.

@johncalvinroberts
Copy link
Contributor Author

After updating, some tests (integration tests?) in readme.ts were failing due to hardcoded non-integer return values. This caused me to notice that French scale implementation was also suffering from the same defect, and could potentially return non-integer score values for slash grades. So, I updated with the same logic, assuming slash grades to be exclusive of adjacent grades in the french scale. Is that a correct?

Yes, now that you point it out, this this is going to be an issue with all the scales. Maybe you could define a function to do this check for each scales. Or I can add it to other scales in a followup.

Updated accordingly with a getRoundedScoreTuple utility func, and applied to other grade scales which could return a tuple with high/low based on adjacent scores. Is that what you had in mind? @musoke

It doesn't feel super DRY, but then again that seems to be the running style in this codebase? The enclosing getScore function is mostly the same across grade scales, so I also tried to follow roughly the same level of abstraction. Let me know if stylistically there is anything I should know 😄

@musoke
Copy link
Collaborator

musoke commented Aug 1, 2023

That looks good to me.

Yes, there is a lot of repetition due to the adhoc evolution of sandbag. The special cases in methods like getScore do make this a little tricky. I would welcome refactors to improve this.

@musoke musoke changed the title disambiguate non-letter'd high yds grades slash grades should have integer scores Aug 1, 2023
@musoke musoke merged commit ab39cd9 into OpenBeta:develop Aug 1, 2023
1 check passed
musoke added a commit to OpenBeta/openbeta-graphql that referenced this pull request Aug 1, 2023
Update to sandbag 0.0.48 to get

- fixed slash grades (OpenBeta/sandbag#143)
- fixed VB grades (OpenBeta/sandbag#147)
- fixed YDS grades > 5.9 with no letter (OpenBeta/sandbag#138)
musoke added a commit to OpenBeta/openbeta-graphql that referenced this pull request Aug 1, 2023
Update to sandbag 0.0.48 to get

- fixed slash grades (OpenBeta/sandbag#143)
- fixed VB grades (OpenBeta/sandbag#147)
- fixed YDS grades > 5.9 with no letter (OpenBeta/sandbag#138)
musoke added a commit to OpenBeta/open-tacos that referenced this pull request Aug 1, 2023
Update to sandbag 0.0.48 to get

- fixed slash grades (OpenBeta/sandbag#143)
- fixed VB grades (OpenBeta/sandbag#147)
- fixed YDS grades > 5.9 with no letter (OpenBeta/sandbag#138)
musoke added a commit that referenced this pull request Aug 1, 2023
#138 forces integer scores for slash grades.  The tests were updated there, but not the examples in the readme.  Update them now.
musoke added a commit that referenced this pull request Aug 1, 2023
#138 forces integer scores for slash grades.  The tests were updated there, but not the examples in the readme.  Update them now.
vnugent pushed a commit to OpenBeta/open-tacos that referenced this pull request Aug 1, 2023
Update to sandbag 0.0.48 to get

- fixed slash grades (OpenBeta/sandbag#143)
- fixed VB grades (OpenBeta/sandbag#147)
- fixed YDS grades > 5.9 with no letter (OpenBeta/sandbag#138)
vnugent pushed a commit to OpenBeta/openbeta-graphql that referenced this pull request Aug 3, 2023
Update to sandbag 0.0.48 to get

- fixed slash grades (OpenBeta/sandbag#143)
- fixed VB grades (OpenBeta/sandbag#147)
- fixed YDS grades > 5.9 with no letter (OpenBeta/sandbag#138)
@johncalvinroberts johncalvinroberts deleted the handle-ambiguous-yds-grades branch August 11, 2023 05:25
l4u532 pushed a commit to l4u532/openbeta-graphql that referenced this pull request Aug 23, 2023
Update to sandbag 0.0.48 to get

- fixed slash grades (OpenBeta/sandbag#143)
- fixed VB grades (OpenBeta/sandbag#147)
- fixed YDS grades > 5.9 with no letter (OpenBeta/sandbag#138)
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.

2 participants