-
Notifications
You must be signed in to change notification settings - Fork 44
config v2 #847
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
base: main
Are you sure you want to change the base?
config v2 #847
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction this would be taking us, and on thinking about it during my review i think we would be better suited to further reduce the top level config options as proposed in #847 (comment)
With my prefered document approach we would have
document:
- key: changelog
mode: append
header:
newlines:
after: 1
- key: news
mode: standalone
header:
newlines:
after: 1
...
project:
- key: project-A
documents:
- changelog
- key: project-B
documents:
- news
- changelog
- key: project-C
documents:
- newsThis config approach would mean that we could have a different combination of docs per project, by default all document templates are used unless it is set.
| b.VersionHeaderPath, | ||
| b.OldHeaderPath, | ||
| b.config.VersionHeaderPath, | ||
| b.cfg.ReleaseNotes.Header.FilePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
| b.cfg.ReleaseNotes.Header.FilePath, | |
| b.cfg.ReleaseNotes.Version.FilePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably, will take a look
|
|
||
| _ = core.WriteNewlines(b.writer, b.config.Newlines.EndOfVersion) | ||
| _ = core.WriteNewlines(b.writer, b.config.Newlines.AfterReleaseNotes) | ||
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Version.Newlines.After) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting was already used and section makes it conceptually clearer.
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Version.Newlines.After) | |
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Section.Newlines.After) |
| _ = core.WriteNewlines(b.writer, b.config.Newlines.EndOfVersion) | ||
| _ = core.WriteNewlines(b.writer, b.config.Newlines.AfterReleaseNotes) | ||
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Version.Newlines.After) | ||
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Newlines.After) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Newlines.After) | |
| _ = core.WriteNewlines(b.writer, b.cfg.ReleaseNotes.Chapter.Newlines.After) |
|
|
||
| l.Project = pc.Key | ||
| projPrefix = pc.Key + config.ProjectsVersionSeparator | ||
| projPrefix = pc.Key + cfg.Project.VersionSeparator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| projPrefix = pc.Key + cfg.Project.VersionSeparator | |
| projPrefix = pc.Key + cfg.Project.Version.Separator |
| for i := range min(int(versionCount), len(vers)) { | ||
| if !firstOutput { | ||
| err := core.WriteNewlines(writer, config.Newlines.AfterChangelogVersion) | ||
| err := core.WriteNewlines(writer, cfg.Changelog.Newlines.After) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming feels out of place should we reuse
| err := core.WriteNewlines(writer, cfg.Changelog.Newlines.After) | |
| err := core.WriteNewlines(writer, cfg.ReleaseNotes.Section.Newlines.After) |
| if cfg.Changelog.Header.FilePath != "" { | ||
| err = core.AppendFile(writer, filepath.Join(cfg.RootDir, cfg.Changelog.Header.FilePath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increase consistency
| if cfg.Changelog.Header.FilePath != "" { | |
| err = core.AppendFile(writer, filepath.Join(cfg.RootDir, cfg.Changelog.Header.FilePath)) | |
| if cfg.ReleaseNotes.Title.FilePath != "" { | |
| err = core.AppendFile(writer, filepath.Join(cfg.RootDir, cfg.ReleaseNotes.Title.FilePath)) |
| } | ||
|
|
||
| _ = core.WriteNewlines(writer, cfg.Newlines.AfterChangelogHeader) | ||
| _ = core.WriteNewlines(writer, cfg.Changelog.Header.Newlines.After) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _ = core.WriteNewlines(writer, cfg.Changelog.Header.Newlines.After) | |
| _ = core.WriteNewlines(writer, cfg.ReleaseNotes.Title.Newlines.After) |
| Change ChangeConfig `yaml:"change"` | ||
| ReleaseNotes ReleaseNotesConfig `yaml:"releaseNotes"` | ||
| Changelog ChangelogConfig `yaml:"changelog"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidated into 1, based on current implementation
| Change ChangeConfig `yaml:"change"` | |
| ReleaseNotes ReleaseNotesConfig `yaml:"releaseNotes"` | |
| Changelog ChangelogConfig `yaml:"changelog"` | |
| ReleaseNotes ReleaseNotesConfig `yaml:"releaseNotes"` |
Or alternatively if implementation switched
| Change ChangeConfig `yaml:"change"` | |
| ReleaseNotes ReleaseNotesConfig `yaml:"releaseNotes"` | |
| Changelog ChangelogConfig `yaml:"changelog"` | |
| Changelog ChangelogConfig `yaml:"changelog"` |
However my preference would be
Or alternatively if implementation switched
| Change ChangeConfig `yaml:"change"` | |
| ReleaseNotes ReleaseNotesConfig `yaml:"releaseNotes"` | |
| Changelog ChangelogConfig `yaml:"changelog"` | |
| Documents DocumentConfig[] `yaml:"document"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still refining all the terms and parts of the config, it was very rough when I started the design coming up on 2 years ago. I do want to do a full pass over on the anatomy of a changelog which is where some of the terms I have been using will need to be refined a bit. Like "a change" versus the "changelog" is not the most helpful at the moment. I don't even think I have enough of the properties in yet and certainly not all the newline spots. So bare with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. What I think would help is to document the composition of the generated output which will help to check and verify the terminology being used.
See comment on body of PR.
| EnvPrefix string `yaml:"envPrefix,omitempty"` | ||
|
|
||
| // TODO: docs | ||
| Fragment FragmentConfig `yaml:"fragment"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also be added to the consolidated property.
| b.cfg.Change.Newlines.Before+1, | ||
| b.cfg.Change.Newlines.After, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| b.cfg.Change.Newlines.Before+1, | |
| b.cfg.Change.Newlines.After, | |
| b.cfg.ReleaseNotes.Change.Newlines.Before+1, | |
| b.cfg.ReleaseNotes.Change.Newlines.After, |
|
Unrelated but I never expected to get any reviews so I didn't have my discord webhook enabled for PR reviews |
|
What would be helpful is to be able to see a visual & textual representation of the composition of a document produced by changie. My thoughts would be the following:
For simplicity purposes this solution would mean we remove the |
Hm, a very different set of terms, here is the layout from the original discussion I created, ignoring the header/footer stuff: Release Notes: Where version, component, kind and fragment are just a format we output. Changelog File: To phrase it similar to you, the list is something along the lines of:
Maybe that is intuitive, not sure, it's kinda late. |
|
So a comparison of the options would be:
What I had missed was multiple projects in 1 document, hence new option c. Personally I don't like using terms like changelog/release notes to describe the parts of the file produced as for me and I am sure other people, they are different things with many differences making them alternatives and not subset. See announcekit.app, releasecat.io, changelogfy, appcues.com, releasenotes.pm etc which highlight my thinking. In my past role for each release I was creating 3 documents each with different audiences:
Also by using generic terms we allow for greater reuse including adr use case etc |
While I appreciate the research and thought put into this. I have a few comments:
|
Closes #848
Check the following