Skip to content

Fixes and improvements when running over non nats competitions - #46

Merged
timreyn merged 5 commits into
cubingusa:mainfrom
viroulep:fixes
Nov 26, 2023
Merged

Fixes and improvements when running over non nats competitions#46
timreyn merged 5 commits into
cubingusa:mainfrom
viroulep:fixes

Conversation

@viroulep

Copy link
Copy Markdown
Contributor

I ran into a couple of crashes when running compscript, here is a description of each change:

  • filename was not defined and made the app crash on startup; I assume it didn't crash because req.session.script was already defined when you introduced it.
  • The competition I used as a sandbox had competitors without results in some events; I chose to return DNS in these cases for the PB, but it can be adjusted to keep returning null and make sure the caller checks for that case. I'm actually unsure about what would be the impact on the other parts of the code so let me know which fix you'd rather keep.
  • The group name assumes that the room is "X Stage" or "Y Room", whereas in French it's usually "Salle X" (or more generally the specific part we want to keep is last in the sentence); I "fixed" the displayed name to just strip Stage and Room from the name.
  • I added a link to open the JSON in a new tab: firefox has a nice json explorer which is quite handy to inspect the script's result (in addition to the already displayed changes of course), and it can also serve as a quick "backup" link. I had never opened a pug template before this, so let me know if there is a cleaner way to do that!
    Additionally the json is currently set in an attribute and passed in the URL, I have no idea how it will scale with "fullsize" competition and an alternate implementation where it's a file download might be needed.

@timreyn timreyn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you!

Comment thread competition.js
Comment thread group.js
Comment thread lib.js Outdated
var matching = person.personalBests.filter((best) => best.eventId === eventId && best.type === type)
if (matching.length == 0) {
return null
return new attemptResult.parseString('DNS')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can make this switch, but I have no idea what it will affect, so I may switch it back later :) There's special handling for null -- most functions automatically return "null" if one of their arguments is "null", unless the argument is "nullable".

One thing that I can think of is ScrambleSpeedScorer in staff/scorers.js -- this uses your PR to determine whether you should be scrambling an event. I think, with this change, it would strongly prefer people with no PR (since -0.02 a very fast PR!) I would suggest fixing this in AttemptResult.valueOf() -- this has special cases that DNF and 0 are very high numbers, but not DNS.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMO that's enough argument to not make this change; for the record it crashed here without the change so I'll update the commit with a check there!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK good call! Yes that is one part that we did not have to worry about for US Nats :)

Stepping back from the code, I think it would be weird to say that your PR in an event is "DNS". So I like null a bit better from that perspective.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the commit fixing this :)

Comment thread parser/parser.js
timreyn added a commit that referenced this pull request Nov 24, 2023
Part of Philippe's #46; I'm going to go ahead and merge is part
separately since it's fixing a breakage.
@viroulep

Copy link
Copy Markdown
Contributor Author

I just pushed a couple more fixes I found while working on the import/export feature:

  • a few Nats Helper leftovers
  • I set a default value for SCRIPT_BASE so that compscript doesn't try to list files in / (obviously I made an arbitrary decision for the default folder, it could be anything else!)
  • I fixed the nested directories listing for the scripts: when I put a json there it crashed trying to list all files in the "folder" scripts/./wcif.json.

@timreyn timreyn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! Just one question, but otherwise good to go.

Comment thread .env.DEV Outdated
API_KEY='example-application-id'
API_SECRET='example-secret'
COOKIE_SECRET='b8f6959746b617d9c9e90ce4a4b6e0'
SCRIPT_BASE=scripts

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm confused by this, why is it searching under '/'? Doesn't listFiles() early-return when this variable isn't set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I got confused, initially when I didn't set SCRIPT_BASE and got no detected scripts I read too quickly the code and thought that it was normal because it was looking at /..

I've removed the commit; there is another discussion over at #52 where it might be needed but it depends on how we want to manage WCIF data so I'll include it there if we need it in the end.

@timreyn
timreyn merged commit a3e4532 into cubingusa:main Nov 26, 2023
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