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

sync: handle docs, metadata, and filepaths #366

Merged
merged 200 commits into from
Dec 15, 2021

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Jun 19, 2021

To-do:

  • Support parsing new options: --docs, --filepaths, --metadata, --tests, -y
  • Rename check.nim to sync.nim, and update.nim to update_tests.nim
  • Clean up update_tests.nim
  • Add initial docs checking
  • Add initial docs updating
  • Add initial metadata checking
  • Add initial metadata updating
  • Add initial filepaths checking
  • Add initial filepaths updating
  • Merge the open PRs and rebase this PR on top
  • (Now on main) test --exercise
  • (Now on main) test --update with y, n, and something else from stdin
  • Consider a separate PR for ff19da4 to help git track the renames. Currently if we squash this PR, git probably doesn't know that sync.nim comes from check.nim. Edit: Let's not bother.
  • Change help message of --exercise
  • Change help message of --yes to clarify that it only affects docs, metadata, and filepaths
  • Change help message of --update
  • Test -y
  • filepaths: don't clone prob-specs when no other scope
  • Make sync -uy and sync -uy --tests produce an error
  • Check the behavior in the edge cases like: exercise config.json doesn't exist, lacks a files key, etc.
  • filepaths, metadata: use only jsony for parsing JSON
  • filepaths, metadata: deserialize without std/json
  • Fix various messages
  • userSaysYes: handle unrecognised entry
  • Only prompt when stderr is a tty
  • jsony: improve error messages, pointing to line + char of JSON parsing error
  • Just include the fix for #31 in this PR
  • docs, filepaths, metadata: improve type safety: enforce that a Slug is kebab-case.
  • docs: fix needlessly slow file comparison
  • docs: resolve the TODO item regarding the top-level header
  • docs: create e.g. instructions.md when it doesn't exist
  • Do a quick profiling pass
  • Optimize away joinPath allocations
  • Improve log readability
  • (Omitting a large number of other, mostly smaller, completed TODO items)
  • tests: json serialization: Test basic serialization and that we serialize every .meta/config.json file on a track the same as parseJson.toPretty() with some keys removed
  • Fix the Windows build
  • Don't create a missing directory until the user confirms
  • Tweak --mode choose|include|exclude into --tests choose|include|exclude
  • Go back to pkg/parsetoml for metadata parsing
  • --filepaths, --metadata: support exercise config containing custom key with the value of any valid JSON object
  • Patch parsing of --tests --foo
  • Fix bug with "custom": {}
  • Add test for problematic CLI parsing case
  • Make configlet sync --filepaths -u and configlet sync --metadata -u preserve key order
  • Reply to Erik's comments
  • Remove fmt code

Final pass:

  • Ensure help message in README.md is up-to-date with the actual configlet -h
  • Update README.md to describe the new sync behavior
  • Run nimpretty
  • Change any new proc to func where possible
  • Only export when necessary
  • Check that doc comments are still correct
  • Check the output of configlet sync on every track

Fixes: #31
Closes: #298


After merging:

  • Tweak key order for configlet fmt?
  • Add configlet fmt
  • Further improve the text in the configlet --help message (I have an unpushed commit that adds a description to every command in the help message).
  • Fix message for missing prob-specs dir (#74)
  • Create a release
  • Update exercism/problem-specifications README.md
  • Update configlet docs in exercism/docs

Then things like:

  • Pin dependency versions
  • Cache prob-specs repo
  • Test --update --docs
  • Test --update --filepaths
  • Test --update --metadata
  • Improve --tests -uy message/interaction further?
  • Maybe make everything an Option[foo] - do we want to have an actual JSON parsing error for things like contributors: null, or e.g. silently replace it on updating?
  • tests: Split the tests of the configlet executable into separate files
  • tests: refactor those tests
  • Maybe don't say "filepaths are up to date". Maybe something like "filepaths are already populated?"
  • --update --exercise foo: improve message consistency
  • --update --yes: put updated summary at the end?
  • --filepaths: warn for invalid placeholders
  • Improve logging and error writing
  • colors: When stdout is a tty, consider [skip] green; [warn] yellow; [error] red
  • Add more messages in verbose mode
  • Refactor to use common write proc
  • Print number of unsynced exercises
  • Hint/warn for e.g configlet sync --metadata -e lasagna (can't sync docs/filepaths/tests for a Concept Exercise)
  • Make --tests use jsony only
  • tests: Test behavior when there are extra key/value pairs
  • Try to refactor some var parameters
  • Remove some cruft from the tests after updating tests.toml in Nim track repo and bumping the Nim commit ref

@ee7 ee7 force-pushed the sync-handle-docs-metadata-filepaths branch from 79ac448 to c7c95c9 Compare June 20, 2021 09:39
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.

Thanks for working on this! I've not done any testing yet, as I think you are still working on this a bit. Let me know when you want me to test.

src/sync/sync.nim Outdated Show resolved Hide resolved
src/sync/sync.nim Outdated Show resolved Hide resolved
src/sync/sync.nim Outdated Show resolved Hide resolved
src/sync/sync.nim Outdated Show resolved Hide resolved
src/sync/sync.nim Outdated Show resolved Hide resolved
src/sync/sync.nim Outdated Show resolved Hide resolved
@ee7
Copy link
Member Author

ee7 commented Jun 23, 2021

@ErikSchierboom If an exercise lacks a config.json file, what should these commands do?

  • configlet sync --filepaths
  • configlet sync --filepaths --update
  • configlet sync --metadata
  • configlet sync --metadata --update

For the --update ones, we want to create the config.json if valid values exist at the upstream source, rather than just complaining that the file doesn't exist?

(Similar to exercism/docs#155 (comment))

@ErikSchierboom
Copy link
Member

For the --update ones, we want to create the config.json if valid values exist at the upstream source, rather than just complaining that the file doesn't exist?

I think that would be nice yeah. It's not a hard requirement, but it would make sense to me.

For the non-update ones, maybe output an error?

@ee7
Copy link
Member Author

ee7 commented Oct 28, 2021

I'll update this PR with the latest changes from main soon.

ee7 added 19 commits October 28, 2021 13:31
I think this is clearer - it's more obvious that we only use the
`testCases` or `testsPath` variable when syncing tests.
The first implementation of metadata syncing produced no output when an
exercise in `problem-specifications` lacked a `metadata.toml` file, and
our tests were using a `problem-specifications` commit where the only
metadata files are YAML.

This commit bumps the `problem-specifications` hash so that our
integration tests have access to `metadata.toml` files.

For the commit that added `metadata.toml` files, see:
exercism/problem-specifications@df6420b279d1
ee7 added 16 commits December 6, 2021 12:13
Previously, a user might think that `-u` does not respect the options
that restrict the data kinds to sync.
We can add this when we test the upcoming `configlet fmt` command.
Let's add this in the commit that implements `configlet fmt`.
This commit guarantees that we don't accidentally call `pretty` in
"format mode" somewhere.

Let's add this code back in the commit that adds `configlet fmt`
instead.

The removed code also had a minor bug, which will be fixed in that
upcoming commit: `fmt` shouldn't write the `editor` key when its value
is the empty array.
This doesn't really matter - it's just that we want to explicitly
initialize `result`, and the previous number could give a misleading
impression of the typical `.meta/config.json` file size.

Across every Exercism track, the mean formatted `config.json` length is
about 430 characters.

The biggest ones are under 1000 characters, so this commit means we'll
(currently) never grow the string, rather than growing it typically 3
times and at most 4 times. This (very marginally) improves performance
when writing `.meta/config.json` files. The number itself optimizes for
the largest config files, not the average ones.
This makes the assertion slightly stronger, by adding the directory at
the start.

It also removes the string allocations due to `lastPathPart`.
This just improves the error message for something that should never
occur, and removes some configlet source file/line number information
from the release executable.

Before this commit:

    $ nimble build -d:danger && strings configlet | grep '.nim('

    @sync_docs.nim(121, 14) `path.endsWith(instrSuffix) or path.endsWith(introSuffix)`
    @sync_filepaths.nim(138, 14) `path.endsWith(&".meta{DirSep}config.json")`
    @sync_metadata.nim(96, 14) `path.endsWith(&".meta{DirSep}config.json")`
We don't need this assertion in the release build.

This commit also finishes removing configlet source file information
from the binary. Before this commit:

    $ nimble build -d:release && strings configlet | grep 'sync_'
    @sync_common.nim(67, 10) `truncateLen <= s.len and s[truncateLen - 1] == DirSep`
This is more readable and explicit.
@ee7 ee7 merged commit 1fd2a46 into exercism:main Dec 15, 2021
@ee7 ee7 deleted the sync-handle-docs-metadata-filepaths branch December 15, 2021 09:40
@ErikSchierboom
Copy link
Member

🥳 Awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants