RobertDober / earmark_parser

The Markdown to AST part of Earmark.
Apache License 2.0
68 stars 26 forks source link

Unneeded deprecation warning for "messages" #133

Closed Sinc63 closed 1 year ago

Sinc63 commented 1 year ago

I think I tracked this down. I upgraded my app from earmark_parser 1.4.13 to 1.4.35. Ran a mix compile and got somewhere around 100 warnings like:

:0: deprecated: messages is an internal option that is ignored and will be removed from the API in v1.5.1 A few comments on the message itself. It isn't particularly helpful to get a message attributed to no_file line 0. If you can get better context that would be great, but otherwise deleting that part would be less frustrating. Second, the message mentions API 1.5.1, but since the message is not attributed to a dependency I ended up having to recursive grep the string in deps. Some of the other deprecation warnings in the app specify EarmarkParser, so doing that here would help a lot. It appears that my only embedded use of EarmarkParser is from PhoenixMarkdown. This is a 5 year old app. That app still refers to Earmark.Options to get or validate the options and I checked: Earmark.Options has the default value of messages set to `[]` rather than the default from EarmarkParser.Options: `MapSet.new([])`. So, we can avoid this noisy warning if you can add another header that looks for `message: []` and returns options, instead of the deprecation warning. I also find that it is often helpful when debugging this sort of thing if the error message prints the value of messages that is unacceptable. That will often make it easier to find where the value originates when it may pass through several levels of parameter. In this case it would have printed `[]` (I think) and really helped with the debugging. It might be beneficial to your own sanity if you could get an update of phoenix_markdown done that uses the latest Earmark/EarmarkParser technology.
RobertDober commented 1 year ago

I am afraid I do not really understand here, if you have hundreds of deprecation messages that means that EarmarkParser.as_ast is called with deprecated values hundreds of times. I cannot see how this can be avoided?

Can you identify the culprit and share this information here?

BTW what is your Earmark version?

RobertDober commented 1 year ago

Maybe you should open an issue in Phoenix Markdown to upgrade to Earmark 1.4 https://github.com/boydm/phoenix_markdown/blob/ef7b5f76f339babec688021080a70708d9ddf1c1/mix.exs#L41

it is quite amazing that Earmark 1.2 and EarmarkParser 1.4 even work together.

Sinc63 commented 1 year ago

Ah, It looks like I have a toxic cocktail.

$ grep earmark mix*
mix.exs:      :earmark,
mix.exs:      {:earmark_parser, "1.4.13", override: true, only: :dev}, # remove when they fix issue 133
mix.lock:  "earmark": {:hex, :earmark, "1.4.15", "2c7f924bf495ec1f65bd144b355d0949a05a254d0ec561740308a54946a67888", [:mix], [{:earmark_parser, ">= 1.4.13", [hex: :earmark_parser, repo: "hexpm", optional: false]}], "hexpm", "3b1209b85bc9f3586f370f7c363f6533788fb4e51db23aa79565875e7f9999ee"},
mix.lock:  "earmark_parser": {:hex, :earmark_parser, "1.4.13", "0c98163e7d04a15feb62000e1a891489feb29f3d10cb57d4f845c405852bbef8", [:mix], [], "hexpm", "d602c26af3a0af43d2f2645613f65841657ad6efc9f0e361c3b6c06b578214ba"},
mix.lock:  "ex_doc": {:hex, :ex_doc, "0.24.2", "e4c26603830c1a2286dae45f4412a4d1980e1e89dc779fcd0181ed1d5a05c8d9", [:mix], [{:earmark_parser, "~> 1.4.0", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "e134e1d9e821b8d9e4244687fb2ace58d479b67b282de5158333b0d57c6fb7da"},
mix.lock:  "phoenix_markdown": {:hex, :phoenix_markdown, "1.0.3", "8095c40dd5037f4b56079ad66de3fe9136406c7c44e1222ce3c74d22e4c7870a", [:mix], [{:earmark, "~> 1.2", [hex: :earmark, repo: "hexpm", optional: false]}, {:html_entities, "~> 0.4", [hex: :html_entities, repo: "hexpm", optional: false]}, {:phoenix, ">= 1.1.0", [hex: :phoenix, repo: "hexpm", optional: false]}, {:phoenix_html, ">= 2.3.0", [hex: :phoenix_html, repo: "hexpm", optional: false]}], "hexpm", "d3591c4cb3357b068cc8284952dbacedb874b287add27787eea2d1d314b18c16"},

Phoenix_markdown only specifies earmark 1.2 or higher, but I have ex_doc pulling in earmark_parser 1.4. But because phoenix is using Earmark.Options to get values it is getting a default of messages: [], which is why I think the change below will help people like me who are hitting this.

(I inserted the mix.exs entry to limit the parser to 1.4.13 to try to avoid all the warnings, but that isn't actually working, and running tests late yesterday I'm getting the smartypants warning as well, from calls to Earmark.as_html!. Sorting that out will be this morning's job. EDIT: I had set env to dev for the old version of earmark_parser to match ex_doc, but I need it generic to avoid issues in test. Somehow this was getting me the old options set and the new deprecations being warned, I guess because test still had the latest parser.)

So, will the phoenix_markdown calling Earmark (1.2) end up using EarmarkParser (1.4), and that's where the trouble comes from?

The fix I was trying to suggest to quiet the warnings is:

  defp _deprecate_old_messages(opitons)
  defp _deprecate_old_messages(%__MODULE__{messages: %MapSet{}} = options), do: options
+  defp _deprecate_old_messages(%__MODULE__{messages: []} = options), do: options

  defp _deprecate_old_messages(%__MODULE__{} = options) do
    %{
      options
      | messages:
          MapSet.new([
            {:deprecated, 0,
             "messages is an internal option that is ignored and will be removed from the API in v1.5.1"}
          ])
    }
  end

Side thought. Removing functions from an API is usually done on a version change of at least the minor version, so that I have a means to avoid it, short of fixing the version statically. Please consider removing things in 1.6 rather than 1.5.1.

Thanks

RobertDober commented 1 year ago

Thank you for the detailed report. I am sorry but in general I (and most developpers) will not change the API to accomodate older incompatible versions, I will however look at what you proposed in case it really helps. For that reason I'll keep the issue open

It seems to me however, that a PR or ticket to PhoenixMarkdown to upgrade Earmark to the latests version is in order.

The problem is here that the main purpose of EarmarkParser is to serve ex_doc and Earmark uses EarmarkParser I was thinking for a long time to decouple them but have hardly the time (and will) to maintain Earmark taking care of EarmarkParser is my main goal

I would love for someone to take over Earmark but I guess I should do the decoupling first 😓

I agree with your assessment for API changes but we are in the 1.4 branch right now, so 1.5 will change the API a little bit, but only according to the deprecations introduced in 1.4

RobertDober commented 1 year ago

I'll try my luck with a ticket to PhoenixMarkdown

RobertDober commented 1 year ago

And also, because not costly, try updating Earmark to 1.4.30 maybe

RobertDober commented 1 year ago

I have looked at your proposed fix. Unfortunately I cannot do this, because this would not allow me to remove the list message API in 1.5.0

I will change the warning to the correct version though, that eluded me before.

I am afraid I have to close this now.

Sinc63 commented 1 year ago

I think that will work out. I will just stick with the old versions of things until you remove the warning about messages, then the old earmark will stop warning for the [] value.

When I get a minute I'll go upvote your phoenix issue.

RobertDober commented 1 year ago

If you can somehow run PhoenixMarkdown with Earmark ~> 1.4.41 you will be save, maybe cloning and using a github mix dependency?