Add published_at to the extracted attributes.

[philipbrown]

I've added extracting the published_at from the article.

[Valian]

Thank you for the contribution, greatly appreciated! Two small comments and one bigger, regarding performance and possible false positives. What do you think?

[Valian]

Isn't it too broad? Floki.text has "deep: true" by default, so what you're doing here is a lot.

Eg. for

<div>
  <p>Hello world</p>
  <div>
    <div>
       <span>Testing</span>
    </div>
  <div>

You're trying to parse dates 5 times, checking the same text multiple times. You can imagine how inefficient it can get with some deeply-nested HTMLs. Not to mention with that approach any valid date is considered published at, which IMO seems a bit too much?

Eg in redability.js published_at seems to be extracted only from meta tags: https://github.com/mozilla/readability/pull/813/files.

Thoughts?

[philipbrown]

Yeah, I agree it's not ideal. There's just so many that don't include it in meta tags or <time> elements, eg. the elixir example in the fixtures.

I did consider making it optional by allowing the consumer to optionally include the date_time_parser package. What do you think? I'm happy to remove it if you'd rather not have it at all though!

[Valian]

@philipbrown I think I'd either remove it or make disabled by default?

Or maybe we can make it use Floki.text(doc, deep: false) by default?

Sth like opts: [find_in_text: :deep | true | false].

Also. Wonder how DateTimeParser works exaclty. If there is a valid date inside the text, will it find it?

If yes, maybe we could take the nested text from the outer-most div and run parser on it?

[philipbrown]

Yeah, I'm just going to remove it. It's more of a nice to have, and it's hard to be confident that it's going to work consistently so I think it's better to just not have it.

[Valian]

Floki.find + flat_map(&Floki.attribute/2) = Floki.attribute/3 . This should simplify these cases.

[philipbrown]

Ah nice 🙏 I didn't realise I could do that! I've pushed a fix for this.

[Valian]

It took me a moment to understand how find_value exactly works with that approach.

Why not

parse(:datetime, value) || parse(:date, value) || parse(:string, value)

or maybe even (just braindumping, not sure about that :D)

with {:error, _} <- DateTime.from_iso8601(value),
     {:error, _} <- Date.from_iso8601(value), 
     {:error, _} <- DateTimeParser.parse(value) do
     nil
else 
     {:ok, datetime, _} -> datetime
     {:ok, datetime} -> datetime
     _ -> nil
 end       

Probably separate functions is clearer and you can use only one of them 😅

[philipbrown]

Yeah you're right, I've changed it to:

parse(:datetime, value) || parse(:date, value) || parse(:string, value)

I'm not a fan of with..else

[Valian]

LGTM 💜 Will release next version as soon as I have a little bit of time, so in a day or two. Thank you @philipbrown !