refactor(config): resolve from the source schema, drop viper from course loading#79
Merged
Merged
Conversation
…rse loading
Switches resolution to the pure resolver added in the previous commit and
deletes the viper-based loader. config/ no longer reads course files through
viper at all: the ~63 viper.GetString("course.assignment.field") calls are gone,
and the only two viper reads left are gitlab.host, which is genuinely global
process configuration.
What this buys, beyond tidiness:
- Resolution no longer mutates global state. The old path wrote the merged
`extends` result back into the viper registry from what was nominally a read,
so results depended on resolution order and two courses could not be resolved
concurrently. config/ tests now run with -race.
- Course names are out of the global keyspace. They used to be merged in
alongside gitlab.host and coursesfilepath, so a course named `courses` would
have collided with the course list.
- A missing course file is an error naming the file, not a panic.
The goldens pass unchanged, which is the whole point: 67 assignments across 8
fixtures resolve identically through a completely different loader. Cross-checked
end to end by diffing `glabs show` for 23 assignments across all five real
courses against a binary built before the switch — identical.
Two validations turned out to be missing from the pure resolver, both caught by
the golden run rather than by the differential test that was supposed to prove
equivalence: startercode without a url, and the rejected `users` key. The
differential test's error list was a hand-copied subset of errors_test.go's — 10
of 13 — and of course the two it omitted were the two that were wrong. Worth
remembering: a test that enumerates cases by hand only covers the cases someone
remembered.
RepoSuffix changes meaning for hand-built configs. It used to consult viper to
decide whether UseEmailDomainAsSuffix had been set at all, so a zero-valued
struct meant "use the domain" even though the field said false. That crutch
cannot survive course files leaving viper, and resolution always sets the field
(absent means true), so the field now simply means what it says. Nothing but the
resolver builds an AssignmentConfig.
The old tests drove config by setting 137 keys on the global viper registry.
They now write the YAML they are testing, which is both closer to what a user has
and readable as configuration rather than as a pile of assignments. Their
coverage is unchanged; the differential test goes, having no second
implementation left to compare against.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Schritt 2b-2 — der Tausch. Baut auf #78 auf (das ist bereits gemergt).
Netto −694 Zeilen. Die ~63
viper.GetString("kurs.assignment.feld")-Aufrufe sind weg; inconfig/bleiben zwei viper-Reads, beide fürgitlab.host— echte Prozess-Konfiguration.Der Beweis
Die Goldens laufen unverändert durch. Das ist der ganze Punkt: 67 Assignments über 8 Fixtures lösen durch einen komplett anderen Loader identisch auf.
Quergeprüft am echten CLI:
glabs showfür 23 Assignments über alle fünf echten Kurse, gegen ein Binary vom Stand vor dem Tausch — identisch.Was das bringt, über Aufgeräumtheit hinaus
extends-Ergebnis in die viper-Registry zurück — aus einem nominellen Lesevorgang. Ergebnisse hingen damit von der Auflösungsreihenfolge ab, und zwei Kurse ließen sich nicht nebenläufig auflösen.config/-Tests laufen jetzt mit-race.gitlab.hostundcoursesfilepatheingemerged — ein Kurs namenscourseshätte mit der Kursliste kollidiert.panic:Ein Fund über meinen eigenen Test aus #78
Zwei Validierungen fehlten im reinen Resolver —
startercodeohneurlund der abgelehnteusers-Key. Gefangen hat sie der Golden-Lauf, nicht der differentielle Test, der die Äquivalenz doch beweisen sollte.Grund: dessen Fehlerliste war eine handkopierte Teilmenge von
errors_test.go— 10 von 13. Und natürlich waren die zwei ausgelassenen genau die zwei, die falsch waren. Merke: ein Test, der Fälle von Hand aufzählt, deckt nur die Fälle ab, an die jemand gedacht hat. Die Goldens haben es gerettet, weil sie nicht aufzählen.Eine Verhaltensänderung, bewusst
RepoSuffixbedeutet für handgebaute Configs jetzt etwas anderes. Es zog früher viper zu Rate, um zu entscheiden, obuseEmailDomainAsSuffixüberhaupt gesetzt war — ein nullwertiges Struct hieß damit „nimm die Domain", obwohl das Feldfalsesagte. Diese Krücke kann nicht überleben, wenn die Kursdateien viper verlassen.Da die Auflösung das Feld immer setzt (abwesend heißt true), ändert sich für aufgelöste Configs nichts. Und
AssignmentConfigwird nirgends von Hand gebaut außer im Resolver selbst (geprüft). Das Feld sagt jetzt endlich, was es heißt.Die Tests
Die alten trieben
config/über 137viper.Set-Aufrufe auf der globalen Registry. Sie schreiben jetzt das YAML, das sie testen — näher an dem, was ein Nutzer wirklich hat, und als Konfiguration lesbar statt als Zuweisungshaufen:Abdeckung unverändert. Der differentielle Test aus #78 fällt weg — es gibt keine zweite Implementierung mehr zum Vergleichen.
Verifikation
go test ./...grün,-racegrün;gofmt,go vet,golangci-lintsauberglabs show23/23 identisch gegen die echten Kursdateienconfig lintweiterhin exit=0,config fmtunverändert🤖 Generated with Claude Code