Updates from Readability version 0.5.0 #46
Conversation
There was a problem hiding this comment.
This LGTM. Thank you for the improvements @yalhyane, really appreciated. This probably overrides #48 and #55 since it's more complete by using the library.
@Monirzadeh can you take a quick look yourself just to have another set of eyes on this? Thank you
parser.go
Outdated
|
|
||
| // DatePublished | ||
| if datePublished, isString := parsed["datePublished"].(string); isString { | ||
| fmt.Println(datePublished) |
There was a problem hiding this comment.
| fmt.Println(datePublished) |
parser-parse.go
Outdated
| if err != nil { | ||
| fmt.Printf("Failed to parse date \"%s\"\n", dateStr) | ||
| return nil |
parser-parse.go
Outdated
| if err != nil { | ||
| fmt.Printf("Failed to parse date \"%s\"\n", dateStr) | ||
| log.Printf("Failed to parse date \"%s\"\n", dateStr) | ||
| return nil |
There was a problem hiding this comment.
what do you think about something like this?
if err != nil {
log.Printf("Failed to parse date \"%s\": %w\n", dateStr, err)
return nil
}
There was a problem hiding this comment.
I was using it to detect unsupported date formats, but I'll delete it.
There was a problem hiding this comment.
He means switching from fmt to log, not deleting it. Log line works fine to display errors in console 👍
There was a problem hiding this comment.
Plus err give us more details.
There was a problem hiding this comment.
My apologies, I misread the code . I also noticed that the Parser has a log method that considers Debug mode. It might be better to make getParsedDate a method of Parser struct so we can use the Parser's log method and print the error in Debug mode only. What do you think?
There was a problem hiding this comment.
Good idea but I think about go-readability do that for all logs.
For example when we use that can define the log mode (in production or debug mode)
But I am not sure if it will be overkill or make things unnecessary complicated.
|
Hey @yalhyane, thank you for the contribution! Just fix the CI error and we can merge this :) |
|
@fmartingr I've fixed it :) |
Awesome, thanks a lot for this! |
Closes #48
Closes #55