-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
- `/theaters` now returns a `application/mill.example.theater` Content-Type header on the following HTTP methods: | ||
- `GET` | ||
- `POST` | ||
- The following Movies resources have changed: |
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 should designate "Movies" with the templating system I have so in Markdown this'll be wrapped in backticks, and in JSON, a mill-changelog
styleable <span>
element.
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 was just about to make that same comment (for all instances of Movies
)
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.
Some suggestions.
- `/theaters` now returns a `application/mill.example.theater` Content-Type header on the following HTTP methods: | ||
- `GET` | ||
- `POST` | ||
- The following Movies resources have changed: |
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 was just about to make that same comment (for all instances of Movies
)
/** | ||
* Get a changelog entry for a changeset that was added into, or removed from, the API. | ||
* | ||
* @param string $definition |
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.
Might not be a problem when looking at the whole, but I needed to do some digging to find an example of a definition, It would be nice to have it a better explanation in these headers.
|
||
$template = $this->templates['singular'][$definition]; | ||
$entry[] = $this->renderText($template, [ | ||
'parameter' => rtrim(ltrim(array_shift($params), '`'), '`'), |
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.
pull this out into a descriptively named function call to allow for explanation without a comment line
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.
Now if only I could remember why I wrote that line originally...
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.
That line of code was actually from when before I had built the Markdown format generator, and it was currently untested. Slightly rewrote the code above it to only templatize $params
if we need to, and added some comments explaining this to futureself.
foreach ($data as /*$uri => */$change_types) { | ||
foreach ($change_types as $change_type => $hashes) { | ||
foreach ($hashes as /*$hash => */$changes) { | ||
if ($definition === 'added' || $definition === 'removed') { |
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.
pull out the constants
* @return string|array | ||
* @throws \Exception If an unsupported definition + change type was supplied. | ||
*/ | ||
private function getEntryForAddedOrRemovedChange($definition, $change_type, array $changes) |
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 feel like the word factory should be used somewhere around here :)
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.
Renaming to getAddedOrRemovedChangesetFactory
and getChangedChangesetFactory
.
trait ChangelogTemplate | ||
{ | ||
/** | ||
* Changelog template output format. Acceptable values: "json", "html", or "markdown" |
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.
feels like those three vals should be const
and referenced instead of compared to strings throughout
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.
Extracted them into:
Changelog::{DEFINITION_ADDED, DEFINITION_CHANGED, DEFINITION_REMOVED}
$searches[] = '{' . $key . '}'; | ||
if (is_array($value)) { | ||
$replacements[] = $this->joinWords( | ||
array_map(function ($val) use ($data_attribute_key, $key) { |
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.
maybe pull this out into a descriptively named function for legibility and to improve reading comprehension?
Also implemented some minor quality of life suggestions from @agilefox.
This makes some updates to the changelog generator to collapse similarly-grouped resources under resource headers, and split apart some other resource action method entries into their own entries.
The end result is hopefully a cleaner, and easier to comprehend changelog.
I also pulled out some changelog changeset code from god-like methods into their own classes. The changelog system as a whole should be easier to maintain now.