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

info: use prob specs cache #605

Merged
merged 1 commit into from
May 20, 2022
Merged

Conversation

ee7
Copy link
Member

@ee7 ee7 commented May 17, 2022

This stops baking the prob-specs data into the binary, and makes
configlet info use the cached prob-specs directory like
configlet sync.

Advantages:

  • The prob-specs data is updated at run time; we no longer need
    commits like b54fe48 or 146d41c, and can remove
    prob_specs_exercises.json
  • The size of the configlet executable is reduced by 12 KiB (2.5%)
  • Fewer lines of code: we can remove bin/write_probspecs_info.nim

Disadvantages:

  • The user must now pass -o or ---offline when running
    configlet info with no/limited network connectivity.
  • configlet info is slower, even when using -o, because we run a few
    git commands to validate the cache repo.

Closes: #483

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Approving the changes in 15ac68d

This stops baking the prob-specs data into the binary, and make
`configlet info` use the cached prob-specs directory like
`configlet sync`.

Advantages:

- The prob-specs data is updated at run time; we no longer need
  commits like b54fe48 or 146d41c, and can remove
  `prob_specs_exercises.json`

- The size of the configlet executable is reduced by 12 KiB (2.5%)

- Fewer lines of code: we can remove `bin/write_probspecs_info.nim`

Disadvantages:

- The user must now pass `-o` or `---offline` when running
  `configlet info` with no/limited network connectivity.

- `configlet info` is slower, even when using `-o`, because we run a few
  `git` commands to validate the cache repo.

Closes: 483
@ee7 ee7 force-pushed the info-use-prob-specs-cache branch from 15ac68d to ad4fbc4 Compare May 17, 2022 09:55
@@ -1,6 +1,6 @@
import std/[algorithm, os, sequtils, sets, strformat, strutils, sugar, terminal]
import pkg/jsony
import ".."/[cli, types_track_config]
import ".."/[cli, sync/probspecs, types_track_config]
Copy link
Member Author

Choose a reason for hiding this comment

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

We could refactor later so that the types/procedures for cloning and validating prob-specs are in a common location.

Copy link
Member Author

@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've checked that, with this PR, the output of configlet info -o is the same on every track as that with configlet info before this PR.

Comment on lines 33 to 40
exerciseFmt*: string
updateFmt*: bool
yesFmt*: bool
of actInfo:
offlineInfo*: bool
of actSync:
exercise*: string
offline*: bool
Copy link
Member Author

Choose a reason for hiding this comment

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

offline and offlineInfo is consistent with e.g. exercise and exerciseFmt. But maybe it's better to move to offlineSync and exerciseSync later.

As a reminder, they can't both be called offline unless we make --offline a global flag. The relevant RFC is nim-lang/RFCs#368.

Copy link
Member

@ErikSchierboom ErikSchierboom May 20, 2022

Choose a reason for hiding this comment

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

Still waiting for the PR to implement that RFC...

Comment on lines +107 to +109
func isOffline(conf: Conf): bool =
(conf.action.kind == actSync and conf.action.offline) or
(conf.action.kind == actInfo and conf.action.offlineInfo)
Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly, more clunky than usual. It'd be nice if something like this were possible:

func isOffline(conf: Conf): bool =
  conf.action.kind in {actInfo, actSync} and conf.action.offline

or just

if conf.action.offline

Copy link
Member

Choose a reason for hiding this comment

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

if conf.action.offline

That would be sweet. We'll just have to wait I guess 🤷

@ee7 ee7 marked this pull request as ready for review May 20, 2022 12:12
@ee7 ee7 merged commit 07a0168 into exercism:main May 20, 2022
@ee7 ee7 deleted the info-use-prob-specs-cache branch May 20, 2022 12:14
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.

info: use a (cached) problem-specifications repo
2 participants