[DOC] Improve NEWS.md rendering and add missing features #9308

zverok · 2023-12-20T21:22:38Z

Fix rendering of the file on docs.ruby-lang.org:
- More indentation for code;
- Properly qualified methods to link to docs (Queue#freeze → Thread::Queue#freeze, remove unnecessary backticks, etc.)
Add two minor yet notable changes:
- Time.new stricter parsing of strings;
- NoMethodError rendering logic change.

jeremyevans

Looks mostly good. Only a couple changes requested.

jeremyevans · 2023-12-20T22:11:03Z

NEWS.md

@@ -67,6 +68,16 @@ Note: We're only listing outstanding class updates.
    * Module#set_temporary_name added for setting a temporary name for a
      module. [[Feature #19521]]

+* NoMethodError


I don't think this change is worth mentioning in NEWS.

I think it is better to explain this in "Compatibility issues" section than in "Core classes updates".

@jeremyevans My thinking here is this:

It is a part of public behavior

It is a behavior that is frequently encountered (i.e., almost every Rubyist sees this error almost every day, while some other notable changes like WeakKeyMap or Fiber#kill are something that wouldn't be used by as many people)

The change affects the quality of life. I understand the justification, yet the change might make somebody's debugging harder (by not seeing the real problematic object) and somebody's easier (by removing 20-lines #inspects of complicated objects)

The change also might affect application/library code (this is purely theoretical, but not unimaginable that some code caught the error and extracted the #inspect of the affected object; or that somebody's #inspect was defined in the way that considered frequent rendering in this message)

The entry in NEWS not only explains the change but links to the corresponding discussion, so when one sees in the new version, "wait, is this changed?" they can a) find the entry in NEWS (and make sure the change was conscious, not some side-effect or bug) and b) find the justification.

@zverok I read what you wrote and still do not think this is worth mentioning. There is a tradeoff in mentioning anything in NEWS. We want NEWS to be short enough that people will read it, hitting the important parts of the release. NEWS is not designed to be comprehensive and discuss every single change since the last release. If you think this is a compatibility issue (I do not), take @mame's advice and put it in the compatibility issues section. Otherwise, I think it should be removed.

I'm for because it is a part of public behavior.
Generally speaking NEWS.md is allowed to write about something if a committer thinks it should be.
I think what jeremy says is rather a release note.

jeremyevans · 2023-12-20T22:11:44Z

NEWS.md

@@ -488,3 +510,5 @@ changelog for details of the default gems or bundled gems.
 [Feature #19965]: https://bugs.ruby-lang.org/issues/19965
 [Feature #20005]: https://bugs.ruby-lang.org/issues/20005
 [Feature #20057]: https://bugs.ruby-lang.org/issues/20057
+[Bug #19293]: https://bugs.ruby-lang.org/issues/19293
+[Feature #18285]: https://bugs.ruby-lang.org/issues/18285


When removing the NoMethodError change, we would want to remove this line as well.

It's better to sort by URLs.

ko1 · 2023-12-21T08:07:43Z

NEWS.md

@@ -75,7 +86,7 @@ Note: We're only listing outstanding class updates.

 * ObjectSpace::WeakMap

-    * `ObjectSpace::WeakMap#delete` was added to eagerly clear weak map


why do you remove backquote even though it is .md?

It will be marked up as <code> without backquotes.

@ko1 Without backticks RDoc is able to linkify the method name.
The current version:

After the fix:

I see.

For me, it is better to allow backquotes and rdoc also accepts backquoted method names in future, because it is easy to copy & paste to other articles.

You mean the case of "only method name enclosed in backquotes", right?
E.g., `ObjectSpace::WeakMap#delete`, but not `(ObjectSpace::WeakMap#delete)`.
It would be better but should be requested to ruby/rdoc, not here.

* Fix rendering of Ruby examples by RDoc; * Fix links to methods references; * Add missing minor changes.

jeremyevans

One additional change requested. After that, OK to merge without further review.

jeremyevans · 2023-12-21T20:24:50Z

NEWS.md

+        ```ruby
+        Time.new('2023-12-20')
+        #  no time information (ArgumentError)
+        Time.new('2023-12-20')


This uses the same code twice, which doesn't appear intentional. I would remove the duplicate lines.

Ugh. Meant to show "year-month" and "year-month-day", it is a typo. But anyway, I think one example is enough.

jeremyevans

I did mention in my previous review that this could be merged without further review after the change I requested was made.

Please squash when merging.

zverok added the Documentation Improvements to documentation. label Dec 20, 2023

zverok requested a review from jeremyevans December 20, 2023 21:22

zverok self-assigned this Dec 20, 2023

jeremyevans requested changes Dec 20, 2023

View reviewed changes

ko1 reviewed Dec 21, 2023

View reviewed changes

zverok added 2 commits December 21, 2023 22:12

Improve NEWS.md rendering and add missing features

f50a004

* Fix rendering of Ruby examples by RDoc; * Fix links to methods references; * Add missing minor changes.

fixup! Improve NEWS.md rendering and add missing features

39539cd

zverok force-pushed the news-missing-features branch from 11427c9 to 39539cd Compare December 21, 2023 20:14

zverok requested a review from jeremyevans December 21, 2023 20:14

jeremyevans requested changes Dec 21, 2023

View reviewed changes

fixup! fixup! Improve NEWS.md rendering and add missing features

608af88

zverok requested a review from jeremyevans December 21, 2023 20:33

jeremyevans approved these changes Dec 21, 2023

View reviewed changes

zverok merged commit fc549b2 into ruby:master Dec 21, 2023
15 checks passed

zverok deleted the news-missing-features branch December 21, 2023 21:12

nurse mentioned this pull request Feb 1, 2024

backport 20172 #9796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOC] Improve NEWS.md rendering and add missing features #9308

[DOC] Improve NEWS.md rendering and add missing features #9308

zverok commented Dec 20, 2023

jeremyevans left a comment

jeremyevans Dec 20, 2023

mame Dec 21, 2023

zverok Dec 21, 2023

jeremyevans Dec 21, 2023

nurse Dec 22, 2023

jeremyevans Dec 20, 2023

nobu Dec 21, 2023

ko1 Dec 21, 2023

nobu Dec 21, 2023

zverok Dec 21, 2023

ko1 Dec 21, 2023 •

edited

nobu Dec 21, 2023

jeremyevans left a comment

jeremyevans Dec 21, 2023

zverok Dec 21, 2023

jeremyevans left a comment

		@@ -75,7 +86,7 @@ Note: We're only listing outstanding class updates.

		* ObjectSpace::WeakMap

		* `ObjectSpace::WeakMap#delete` was added to eagerly clear weak map

[DOC] Improve NEWS.md rendering and add missing features #9308

[DOC] Improve NEWS.md rendering and add missing features #9308

Conversation

zverok commented Dec 20, 2023

jeremyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ko1 Dec 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyevans left a comment

Choose a reason for hiding this comment

ko1 Dec 21, 2023 •

edited