Skip to content

Conversation

@micvbang
Copy link
Contributor

Hey,

I needed to use header information in my consumption of the API.

I suggest to return only a copy of the header since information in it is used during demo parsing, i.e. Progress.

Best,
Michael

@micvbang
Copy link
Contributor Author

micvbang commented Apr 14, 2018

Ah. I had actually completely missed the HeaderParsedEvent. Maybe this addition does not make sense anyway?

@markus-wa
Copy link
Owner

markus-wa commented Apr 15, 2018

I think it's quite weird to use the event for header information tbh, not sure why I put it there.

I suggest removing the HeaderParsedEvent completely and returning a copy of the header in ParseHeader() (for ease of use). I would also like to keep your Header() function so the header doesnt need to be saved preemptively.

But in turn I'd suggest removig the helper functions like Parser.Map().

What do you think?

@markus-wa
Copy link
Owner

markus-wa commented Apr 15, 2018

I'm also wondering why Parser.header is a pointer. Will check later

So it can be nil

@micvbang
Copy link
Contributor Author

Yes, I agree.

Since this is a library used by more people, I would suggest not removing the HeaderParsedEvent. It's likely that someone depends on it, and that removing it would break their code :)
As far as I understand, the way to change the API in Go libraries is to release them under a new URI. Please correct me if you think otherwise!

If you haven't done this before I get to work on it during next week, feel free do do the work on my PR. Also, if you have any code that you'd like reviewed, feel free to make a PR and assign it to me.

@markus-wa
Copy link
Owner

markus-wa commented Apr 15, 2018

I think that today, with tools like dep, changes like this shouldn't be a problem. This lib uses SemVer and as of now we're still at v0.x.x.

As a compromise (to not break peoples code if they don't use dep yet) I'd go for deprecating it (via comment) and removing it in v1.0.0.

Also, if you have any code that you'd like reviewed, feel free to make a PR and assign it to me.

Thanks! Will keep it in mind

Repository owner deleted a comment from codecov bot Apr 15, 2018
* Return header in ParseHeader()
* Deprecated HeaderParsedEvent (GoDoc)
* Deprecated Parser.Map()
Repository owner deleted a comment from codecov bot Apr 15, 2018
@markus-wa
Copy link
Owner

Let me know if you think this looks good and I'll merge it.
I already took the liberty of rebasing it onto master.

Repository owner deleted a comment from codecov bot Apr 15, 2018
@micvbang
Copy link
Contributor Author

Looks good!

@markus-wa markus-wa merged commit fab5fce into markus-wa:master Apr 16, 2018
Repository owner deleted a comment from codecov bot Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants