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

fmt, info, lint, sync: extract common types #564

Merged
merged 3 commits into from
May 10, 2022

Conversation

ErikSchierboom
Copy link
Member

@ErikSchierboom ErikSchierboom commented Apr 15, 2022

Feel free to change the commit message whatever you like

Closes: #558

@ErikSchierboom ErikSchierboom changed the title Refactor formatting refactor: extract types exercise_config and track_config to separate files Apr 15, 2022
@ErikSchierboom ErikSchierboom requested a review from ee7 April 15, 2022 13:39
src/sync/sync_common.nim Outdated Show resolved Hide resolved
src/types_track_config.nim Show resolved Hide resolved
src/lint/track_config.nim Outdated Show resolved Hide resolved
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.

There's some subtlety in the slug change.

The idea was that we only initialize a Slug with a known kebab-case value. With the current state, lint now uses Slugs that may not be kebab case.

We could move the postHook from sync_common to types_track_config, make other things Slug, and then remove some kebab-case checking in lint. But that's technically a behavior change for lint - it would exit on the first non-kebab slug without reporting any other problems. And this PR is complex enough anyway, so let's leave everything else for later. We'll also try to make fmt not import sync stuff.

I'll merge soon, I just want to:

  • Do a sanity check of the lint/sync output on every track
  • Briefly consider reverting everything from the commit I added apart from the $ changes.

Also for later: there's a pre-existing bug in tidyJsonyErrorMsg - it shouldn't say "configlet lint", since it isn't used only by lint. See 6b215a3 for background.

ErikSchierboom and others added 3 commits May 10, 2022 14:00
Previous commits in this series moved some types to a common file. This
meant that, for example, `info.nim` no longer needed to import
`lint/track_config` to import the types. However, it still needed to
import it for the `$` and the `init` procedures.

This commit improves a little, moving the minimum procedures to:
- Deduplicate the `$` funcs that existed in both `lint/track_config`
  and `sync/sync_common`.
- Allow `info.nim` to no longer import `lint/track_config`.

The name "types_track_config" is perhaps misleading now, however (since
it's not _just_ types). But I think it's an improvement overall.
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.

This PR preserves the output of lint and sync on every track.

Resolved merge conflict due to 152a953.

Branch diff before force-push: https://github.com/exercism/configlet/compare/4.0.0-beta.1..e079de9834f80b8c01f5dbb0d23b7f5cde50fd0a

Branch diff after force-push: https://github.com/exercism/configlet/compare/4.0.0-beta.2..23cad1fee0e2890423d247ec6e9dd900991581f8

$ git diff 4.0.0-beta.1 e079de98 | grep '^[+-]' > /tmp/before.txt
$ git diff 4.0.0-beta.2 23cad1fe | grep '^[+-]' > /tmp/after.txt
$ git diff /tmp/before.txt /tmp/after.txt > /tmp/diff_between_diffs.txt
diff --git a/tmp/before.txt b/tmp/after.txt
--- a/tmp/before.txt
+++ b/tmp/after.txt
@@ -84,6 +84,7 @@
 -    example*: seq[string]
 -    exemplar*: seq[string]
 -    editor*: seq[string]
+-    invalidator*: seq[string]
 -
 -  TrackConfig* = object
 -    slug*: string
@@ -179,6 +180,7 @@
 -    exemplar*: seq[string]
 -    example*: seq[string]
 -    editor*: seq[string]
+-    invalidator*: seq[string]
 -
 -  TrackConfig* = object
 -    exercises*: Exercises
@@ -229,6 +231,7 @@
 -  #     solution*: seq[string]
 -  #     test*: seq[string]
 -  #     editor*: seq[string]
+-  #     invalidator*: seq[string]
 -  #     case kind*: ExerciseKind
 -  #     of ekConcept:
 -  #       exemplar*: seq[string]
@@ -261,6 +264,7 @@
 -    fkExemplar = "exemplar"
 -    fkExample = "example"
 -    fkEditor = "editor"
+-    fkInvalidator = "invalidator"
 -
 -  ConceptExerciseFiles* = object
 -    originalKeyOrder: seq[FilesKey]
@@ -268,6 +272,7 @@
 -    test*: seq[string]
 -    exemplar*: seq[string]
 -    editor*: seq[string]
+-    invalidator*: seq[string]
 -
 -  PracticeExerciseFiles* = object
 -    originalKeyOrder: seq[FilesKey]
@@ -275,6 +280,7 @@
 -    test*: seq[string]
 -    example*: seq[string]
 -    editor*: seq[string]
+-    invalidator*: seq[string]
 -
 -  ConceptExerciseConfig* = object
 -    originalKeyOrder: seq[ExerciseConfigKey]
@@ -360,6 +366,7 @@
 +  #     solution*: seq[string]
 +  #     test*: seq[string]
 +  #     editor*: seq[string]
++  #     invalidator*: seq[string]
 +  #     case kind*: ExerciseKind
 +  #     of ekConcept:
 +  #       exemplar*: seq[string]
@@ -392,6 +399,7 @@
 +    fkExemplar = "exemplar"
 +    fkExample = "example"
 +    fkEditor = "editor"
++    fkInvalidator = "invalidator"
 +
 +  ConceptExerciseFiles* = object
 +    originalKeyOrder*: seq[FilesKey]
@@ -399,6 +407,7 @@
 +    test*: seq[string]
 +    exemplar*: seq[string]
 +    editor*: seq[string]
++    invalidator*: seq[string]
 +
 +  PracticeExerciseFiles* = object
 +    originalKeyOrder*: seq[FilesKey]
@@ -406,6 +415,7 @@
 +    test*: seq[string]
 +    example*: seq[string]
 +    editor*: seq[string]
++    invalidator*: seq[string]
 +
 +  ConceptExerciseConfig* = object
 +    originalKeyOrder*: seq[ExerciseConfigKey]
@@ -477,6 +487,7 @@
 +    exemplar*: seq[string]
 +    example*: seq[string]
 +    editor*: seq[string]
++    invalidator*: seq[string]
 +
 +  Concept* = object
 +    name*: string

@ee7 ee7 merged commit 85e262c into exercism:main May 10, 2022
@ee7 ee7 changed the title refactor: extract types exercise_config and track_config to separate files fmt, info, lint, sync: extract common types May 10, 2022
Copy link
Member Author

@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 getting this merged!

Comment on lines +49 to +51
# (again because the JSON `files` data does not contain a `kind` key) we can
# theoretically determine the `kind` by the presence of the `exemplar` or
# `example` keys.
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you hook into jsony somehow?

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.

fmt, info, lint, sync: use common types
2 participants