Skip to content

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented Mar 6, 2024

See erlang/otp#7720

Following #13392

Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits but looks great. 💯

@@ -530,6 +530,34 @@ defmodule Logger.TranslatorTest do
assert {%RuntimeError{message: "oops"}, [_ | _]} = process_metadata[:crash_reason]
end

if :erlang.system_info(:otp_release) >= ~c"27" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got it too 🙃

Suggested change
if :erlang.system_info(:otp_release) >= ~c"27" do
if System.otp_release() >= "27" do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to do the following:

@tag skip: System.otp_release() < "27"

(or if ..., do: @tag :skip.)

because it's less indent and less git diff when it's ultimately gonna be enabled. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure also works!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's neat! Thanks @whatyouhide @wojtekmach !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got it too 🙃

We need to tell Jose 😄
7291154

sabiwara and others added 2 commits March 6, 2024 20:44
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
@sabiwara sabiwara merged commit 15586e2 into elixir-lang:main Mar 6, 2024
@sabiwara sabiwara deleted the logger-otp27 branch March 6, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants