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

Update average run time #430

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Update average run time #430

merged 2 commits into from
Jul 18, 2023

Conversation

ErikSchierboom
Copy link
Member

@ErikSchierboom ErikSchierboom commented Feb 15, 2023

There are two reasons for this change:

  1. Having the average run time as a float gives the impression of being exact, whereas the actual run time wildly varies due to a wide variety of reasons (e.g. how busy it is on the server). That fractional component will almost never actually conform the real situation.

  2. jq is often used to work with track config.json config files (e.g. to add elements to it), and it will remove any trailing .0 fractional part from a number, which caused configlet lint to fail. Those JQ scripts then have to work around this by manually adding .0 to it.

Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

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

I don't love this, since I think all the other values have exactly one valid JSON type. But it's probably not too annoying for configlet lint, and I can agree that getting an error for 5 feels a bit painful.

An alternative is to move to integer only. But:

  • that requires an Exercism-wide PR, which probably isn't worth our time.
  • if a track gets down to very low average run times, the difference between e.g. 1.0 and 1.5 may be meaningful enough.

So I'm OK with this PR. But I think I prefer it as float only. I think configlet lint would need some custom parsing hook to not error when it sees an integer when it's expecting to set the value of a float field. And it's not like tracks are often seeing an error for an incorrect type here, right?

Are there any other values where we want to be more permissive with the type? But e.g. we don't accept a value of "3" for a version, even though we could argue that "we know what the user means".

@ee7
Copy link
Member

ee7 commented Feb 15, 2023

Also, when configlet fmt supports the track-level config.json file, this PR raises the questions:

  • should configlet fmt exit non-zero when the value is an int?
  • when -u is passed, should configlet fmt rewrite e.g. 5 as 5.0, or vice versa?

The answer to both questions should be the same, because configlet fmt -u should only format things that configlet fmt complains about.

So we have two options:

  • Both answers are "no", then configlet fmt has to parse e.g. 5 as a float, but later remember that it was previously an integer, and not write it as 5.0. That's awkward.
  • Both answers are "yes", then there's a de-facto canonical representation of average_run_time as either a float or an int. I expect that eventually most tracks will enable configlet fmt in CI, which means there's no benefit on being lenient in configlet lint anyway - it'll still fail CI. And it's worse, because the user only sees the problem at CI-time, not when running configlet lint locally before making a PR.

Things are simpler if the type of everything is known at compile time. So I think this PR isn't worth it. Thoughts?

@ErikSchierboom
Copy link
Member Author

that requires an Exercism-wide PR, which probably isn't worth our time.

Well, why not? Should be easy enough.

if a track gets down to very low average run times, the difference between e.g. 1.0 and 1.5 may be meaningful enough.

It won't be though, as there are many other factors that influence the average run time, like how crowded our servers are. People won't notice a difference of half a second.

building/configlet/lint.md Outdated Show resolved Hide resolved
This was referenced Jul 14, 2023
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ErikSchierboom ErikSchierboom merged commit 56120d3 into main Jul 18, 2023
@ErikSchierboom ErikSchierboom deleted the update-average-run-time branch July 18, 2023 08:48
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.

4 participants