TECH-4252: support native add for broadcast #14
Conversation
CHANGELOG.md
Outdated
| `**context` argument, if present. If that argument is not present (such as with `broadcast`), Ruby will instead | ||
| match the `**context` up to the `progname` argument. | ||
|
|
||
| ## [0.5] - 2019-03-06 |
There was a problem hiding this comment.
Can these be made into diff links so that it's easy to jump to the change diff in the code? Example for this one would be to add to the bottom of the file
[0.5]: https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0
There was a problem hiding this comment.
Yes, they could. Are you suggesting we always write those?
There was a problem hiding this comment.
Yea it's very helpful to link from the version to the code change diff. If you follow the Changelog spec that you're linking to, link pointers are accumulated at the bottom of the file, and the version number is what links to the change diff, rather than a diff hyperlink adjacent to it.
There was a problem hiding this comment.
I followed the Changelog spec as far as "There is no standard format." And then I clicked on their exemplar project which led me here:
https://github.com/olivierlacan/keep-a-changelog/blob/master/CHANGELOG.md
If we're going to link to diffs, might as well link to them where they're most relevant. Given the most-recent-first convention, people aren't going to think to scroll to the bottom. I sure didn't.
| `**context` argument, if present. If that argument is not present (such as with `broadcast`), Ruby will instead | ||
| match the `**context` up to the `progname` argument. | ||
|
|
||
| ## [0.5] - 2019-03-06 [diff](https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0) |
There was a problem hiding this comment.
keepachangelog.com shows a neat way of doing this in their CHANGELOG... It would look like this:
| ## [0.5] - 2019-03-06 [diff](https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0) | |
| ## [0.5] - 2019-03-06 | |
| ### Added | |
| - Extracted normalize_log_level to a public class method so we can call it elsewhere where we allow log_level to be | |
| configured to text values like 'debug'. | |
| [0.5]: https://github.com/Invoca/contextual_logger/compare/v0.4.0...v0.5.0 |
i.e. It allows for in-line more readable lines that reference to defined links at the bottom of the file.
There was a problem hiding this comment.
Wouldn't we prefer the version to be [0.5.0] here?
There was a problem hiding this comment.
Yes, while they're equivalent, I agree that the extra .0 is more clear... so I've added it.
There was a problem hiding this comment.
i.e. It allows for in-line more readable lines that reference to defined links at the bottom of the file.
I thought diff was pretty readable? No one needs to see the complete github URL--that's irrelevant. And scrolling to the bottom seems like a problem for discoverability. I didn't discover it in their example! And for their github example, it looks like they buried the diff in a hyperlink of the version number. I tried that first, but rejected it for poor discoverability.
There was a problem hiding this comment.
Ah i think I might know where confusion is coming in here. The original spec is using reference style linking for the version headers. By making the headers the version number with surrounding brackets (ex. [0.0.1]), it’s using link reference syntax to turn that version number into a link by defining the link at the bottom of the file with [0.0.1]: https//.... The references at the bottom of the file are not rendered by markdown. This exists to help with readability when viewing the actual markdown, like you would when pulling the repo locally and reading.
You can see this in action when you look at the raw markdown of the spec example
There was a problem hiding this comment.
Ah, ok, I didn't know about that notation. That explains why the links are at the bottom of the source. But that seems needlessly complex to manage (add your new version to the top of the file, but don't forget to go near the bottom to add the supporting reference links... and make sure the version numbers are named exactly the same [see 0.5 vs. 0.5.0] so markdown can join them up).
And this still has the same issues around discoverability. I wouldn't expect the version number to link to a raw github diff. The very point of the CHANGELOG is to summarize the changes so you don't have to look at the raw diff! It should be very rare that you'd want to drill into the diff.
There was a problem hiding this comment.
But that seems needlessly complex to manage
It's also dry. If for any reason there is the need to reference an older version, and you'd like to specifically link to it, you simply add brackets around the version number and this turns it into the same link.
and make sure the version numbers are named exactly the same [see 0.5 vs. 0.5.0]
The versions in the headers should be following semver, so it shouldn't be too much to keep them in sync. In semver 0.5 and 0.5.0 do actually mean different things. 0.5.0 is an exact version, and 0.5 is a pseudo version meant to reference the latest stable version under 0.5. So if 0.5.1 was released, 0.5 would point semantically to 0.5.1 not 0.5.0. This is why the spec uses the full versions and not pseudo versions.
The very point of the CHANGELOG is to summarize the changes so you don't have to look at the raw diff
I completely agree. And that's exactly why the diff is added as a hyperlink and pushed out of the way in the markdown. The diff is unimportant and supplementary if anything. So it's made sure to be completely out of the way when reading the rendered markdown (through the use of a link), or reading the raw markdown (by using link reference notation).
There was a problem hiding this comment.
It's also dry. If for any reason there is the need to reference an older version, and you'd like to specifically link to it, you simply add brackets around the version number and this turns it into the same link.
Nah, you can't count hypothetical future cases as a DRY plus today. (YAGNI) Today it's actually less DRY, because you need to repeat the version numbers in order for the join to work. For example, when I renamed 0.5 to 0.5.0 it would have been easy to not remember to go to the bottom of the file and rename there too.
0.5 is a pseudo version meant to reference
I didn't realize that "pseudo versions" were a formal concept. I know about those in the Gemfile version matchers like ~> 0.5, but I was under the impression that 0.5 and 0.5.0 were exactly the same version. Do you have any references you can point me to about "pseudo-versions" in Ruby?
Doesn't this apply one level deeper? Aren't 0.5.1.0 and 0.5.1 the same version?
I don't think you addressed my "discoverability" comment. How would you expect a reader to know that the version hyperlink goes to a raw github diff? I expected it to go to exactly that version.
There was a problem hiding this comment.
Nah, you can't count hypothetical future cases as a DRY plus today.
I'm not arguing DRY for the sake of this implementation. I'm stating it as the reasoning for this standard with 76 contributors and used by many many open source projects being the way it is.
I didn't realize that "pseudo versions" were a formal concept
I haven't found any specific documentation for ruby, but I don't know why ruby matters. Semver is for all languages. If you look at Docker, releases work the same way for semver versioning of images. Pulling theruby:2.5image will get you the latest patch version of version2.5. Same with python, ubuntu, etc. The open source community is standardizing around this concept.
All and all, I think that we should follow the standard. If we done, you should remove the reference to the standard from the Changelog. It's more confusing to say "We follow X standard" and then change it.
|
This has been merged and released it looks like, so closing the PR |
Fix for TECH-4252.
Summary of Changes
Matched up the interface to
Logger#addsince Rails replaces with a mixin when you callbroadcast, as Rails::Server does. See CHANGELOG for more detail. :)