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

Add titles to the metadata for all exercises #2122

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Conversation

kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Oct 9, 2022

Only a handful of exercises had titles.
These were typically exercises where the correct title would be difficult to guess based on the slug (e.g. D&D Characters).

However, it turns out that many exercises have variations on the titles across different tracks.

This hard-codes titles for all the exercises, even if they would technically be fairly straight forward to derive from the slug.

This will make it easier for Configlet to do normalization on the track config.json.

exercises/nucleotide-count/metadata.toml Outdated Show resolved Hide resolved
exercises/octal/metadata.toml Outdated Show resolved Hide resolved
Only a handful of exercises had titles.
These were typically exercises where the correct title would
be difficult to guess based on the slug (e.g. D&D Characters).

However, it turns out that many exercises have variations on
the titles across different tracks.

This hard-codes titles for all the exercises, even if they
would technically be fairly straight forward to derive from
the slug.

This will allow us to have configlet normalize titles.
@kytrinyx kytrinyx merged commit 6e57ab4 into main Oct 11, 2022
@kytrinyx kytrinyx deleted the exercise-titles branch October 11, 2022 07:51
@ee7
Copy link
Member

ee7 commented Oct 11, 2022

I like the idea to ensure that the name of a prob-specs-derived practice exercise in a track-level config.json file matches that in prob-specs.

For example, we'd change the title of DnD Character in:

to match the title of D&D Character in all the other implementations:

But what was the intention for configlet? The options I see immediately are:

  1. Check every exercises.practice.name during configlet lint, and emit an error for a non-match.
  2. Check every exercises.practice.name during configlet lint, and emit a warning if it doesn't match.
  3. Make configlet sync --metadata exit non-zero if the track-level config.json exercises.practice.name value does not match the corresponding exercise title in prob-specs, and update it with configlet sync --update --metadata (which previously only touched the exercise-level config.json)

@kytrinyx
Copy link
Member Author

I will submit PRs to all tracks this week to make them match the title in problem specifications.

Once we know that all tracks are in compliance, I think it would make sense to do 1 & 3.

@ee7
Copy link
Member

ee7 commented Oct 11, 2022

Currently, the metadata.toml values are written to exercise config.json files (for example, here, and configlet sync keeps them synced.

What should we do with the new title keys? I can see these options:

a. Add the title key to the exercise config.json file, and remove the corresponding exercises.practice.name key from the track-level config.json file. And move all the exercises.concept.name keys to corresponding exercise config.json files too.
b. Add the title key to the exercise config.json file, but keep the corresponding exercises.practice.name key in the track-level config.json file (DRY violation, so probably bad).
c. Do not add the title key to the exercise config.json file, keeping it only in the track config.json file.

Without thinking about it much, my preference is option a so that configlet sync --metadata only syncs/updates exercise config.json files. But I don't know if that's worse for the website, or something else. Thoughts?

@kytrinyx
Copy link
Member Author

From a tool-maintainer perspective I would also instinctively want to do (a), but from a product point of view I would prefer (c), as the track config.json has more to do with how things fit together at a high level and how the track is displayed on the site.

I think I would prefer for sync to either to fix the title value in the track-level config.json or do nothing.

From the perspective of someone using configlet to help bootstrap a track, I think the most helpful thing would be to have a separate configlet init command that takes the --exercise argument and assumes that you have not done anything yet to start working on that exercise. It would then

  1. Write an entry to the track-level config.json with all the required fields, adding the value for slug and title and adding a new uuid.
  2. Sync the exercise docs and metadata (without asking for confirmation)
  3. Add empty files for all the files specified in the config.json

I'm not sure what the ideal behavior would be around the tests. Probably you'd want to either ask for confirmation for each test, but maybe just defer the problem and have people sync the tests separately.

This would remove a lot of the fiddly bits of getting started with a new exercise, even if the track doesn't have a generator, and then we could assume that the title is correct for the purposes of the sync command, and just error out in the lint command if it is wrong for some reason.

@ee7
Copy link
Member

ee7 commented Oct 11, 2022

From a tool-maintainer perspective I would also instinctively want to do (a), but from a product point of view I would prefer (c), as the track config.json has more to do with how things fit together at a high level and how the track is displayed on the site.

OK.

From the perspective of someone using configlet to help bootstrap a track, I think the most helpful thing would be to have a separate configlet init command

I've also thought about this. Could you please open an issue in the configlet repo? We'd want it to work for both concept exercises and practice exercises, right? Then I think we'd want a --kind concept|practice option or similar.

  1. Sync the exercise docs and metadata (without asking for confirmation)

And filepaths, right?

then we could assume that the title is correct for the purposes of the sync command

Ideally, I think sync should still handle syncing the title. The title could change in prob-specs due to a typo/bad capitalization/style, or because the name of the exercise changed, right? We'd want to propagate that upstream change.


Edit:

and just error out in the lint command if it is wrong for some reason.

Mmm. First reaction: I think we shouldn't do this, because we don't want a title value changing in prob-specs to cause track CI to fail, right? Similarly, configlet lint does not currently indicate an error if an exercise blurb/source/source_url is unsynced. I think we should leave it to configlet sync, and make configlet sync run in track CI and print warnings. What do you think?

@kytrinyx
Copy link
Member Author

Could you please open an issue in the configlet repo?

Yes, I'll do this now.

And filepaths, right?

Yes, and filepaths.

Ideally, I think sync should still handle syncing the title. The title could change in prob-specs due to a typo/bad capitalization/style, or because the name of the exercise changed, right? We'd want to propagate that upstream change.

Hm, yes, on reading this my conclusion is that you're right.

So the conclusion so far is:

  • we want a separate init or bootstrap or something command, which I will open an issue for now, and
  • lint shouldn't error
  • sync should update the title if there is a mismatch

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.

6 participants