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

Clean up tech debt in YAML unmarshaling #30

Closed
drevell opened this issue Jun 15, 2023 · 0 comments · Fixed by #161
Closed

Clean up tech debt in YAML unmarshaling #30

drevell opened this issue Jun 15, 2023 · 0 comments · Fixed by #161
Labels

Comments

@drevell
Copy link
Contributor

drevell commented Jun 15, 2023

TL;DR

Our current YAML parsing works, but there might be a less-boilerplatey way to do YAML unmarshaling while:

  • enforcing required fields
  • enforcing that no extra unknown fields were present
  • doing custom validation on field values

The upstream YAML parser has an interesting bug go-yaml/yaml#460 where it will enforce that no extra fields are present, but that feature doesn't work if you call node.Decode(). That may limit our options for doing this cleanly.

Example of problematic code: in spec.go, there is some repeated almost-copy-pasted boilerplate code that looks like this:

// UnmarshalYAML implements yaml.Unmarshaler.
func (s *Spec) UnmarshalYAML(n *yaml.Node) error {
	knownYAMLFields := []string{"apiVersion", "kind", "desc", "inputs", "steps"}
	if err := extraFields(n, knownYAMLFields); err != nil {
		return err
	}

	type shadowType Spec
	shadow := &shadowType{} // see "Q2" in file comment above
	if err := n.Decode(shadow); err != nil {
		return err
	}

	*s = Spec(*shadow)
	s.Pos = yamlPos(n)
	return nil
}

Copy-pasted code like this is almost always harmful.

Detailed design

No response

Alternatives considered

No response

Additional information

No response

@drevell drevell added enhancement New feature or request cleanup and removed enhancement New feature or request labels Jun 15, 2023
drevell added a commit that referenced this issue Aug 22, 2023
Before this, each UnmarshalYAML function was a copy-pasted block of
20-ish lines of boilerplate. With this change, each UnmarshalYAML
function turns into a one-liner which calls unmarshalPlain.

This requires a somewhat regrettable amount of reflection code, but it's
arguably no more confusing than what was there before, while having the
benefit of being more concise and DRY.

Fixes #30.
drevell added a commit that referenced this issue Aug 24, 2023
Before this, each UnmarshalYAML function was a copy-pasted block of
20-ish lines of boilerplate. With this change, each UnmarshalYAML
function turns into a one-liner which calls unmarshalPlain.

This requires a somewhat regrettable amount of reflection code, but it's
arguably no more confusing than what was there before, while having the
benefit of being more concise and DRY.

Fixes #30.

I'm attempting to use stacked PRs here; I intend to switch the base
branch of this PR to `main` once the `drevell/for-each-values-from`
branch is merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

1 participant