Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support passing http.Header to exported functions #37

Merged
merged 7 commits into from
Apr 18, 2022

Conversation

nathj07
Copy link
Contributor

@nathj07 nathj07 commented Apr 1, 2022

In doing this we allow the user of the library to pass nil, getting the current default behaviour, or
specify a header. This header can then be used to indicate to the Tika server the response format that is required.

This should be useful to folks because Tika can respond with HTML, plain text, or json. At this point I've not added
any validation in here. My assumption is that the Tika server will handle that.

I updated the tests to nimply use nil for this new parameter. If it is wanted I could look to add some new tests that
exercies different response types. It may also be nice to update the cli tool to allow the user to indicate the
type of response required. Given the definition of the issue though that felt out of scope here.

I hope this is helpful.

In doing this we allow the user of the library to pass nil, getting the current default behaviour, or
specify a header. This header can then be used to indicate to the Tika server the response format that is required.

This should be useful to folks because Tika can respond with HTML, plain text, or json. At this point I've not added
any validation in here. My assumption is that the Tika server will handle that.

I updated the tests to nimply use nil for this new parameter. If it is wanted I could look to add some new tests that
exercies different response types. It may also be nice to update the cli tool to allow the user to indicate the
type of response required. Given the definition of the issue though that felt out of scope here.

I hope this is helpful.
@nathj07
Copy link
Contributor Author

nathj07 commented Apr 1, 2022

I have a small tool I'm using this in locally. I've been able to use the code on this PR and verify the results are as I expected.

@tbpg
Copy link
Member

tbpg commented Apr 18, 2022

Thanks! I'm good to add support for headers, but I'd prefer not to make a breaking change to the API. Just to throw something out there, what if we had something like Client.WithHeader? The issue with that is not every request from the same client should have the same headers. So, WithHeader would maybe need to return a copy?

@nathj07
Copy link
Contributor Author

nathj07 commented Apr 18, 2022

I'll do that for sure, makes total sense. It'll be sometime this week that I get to it.

I should've done a WithHeader version. Thanks for the feedback

This adds two new Parse functions, ParseWithHeader and ParseReaderWithHeader.
I've also added a specific test case here for the ParseWithHeader function to ensure it
is using the header correctly.

The other change here, which no longer breaks the API, is to have the original Parse and
ParseReader simply call the new WithHeader versions setting a nil header.
This approach means that we still only have one place where call and callString are called
but that we also have the ability to have headers or not.
@nathj07
Copy link
Contributor Author

nathj07 commented Apr 18, 2022

Instead of Client.WithHeader I've simply added two extra Parse functions. It would be possible to have a Headers field on the Client struct but that felt a little clunky given that a number of internal/private functions already have a header http.Header parameter.

Hopefully what I've done makes sense here. It no longer breaks the API, doesn't require the caller to have multiple copies of the client for different headers, and it should be clear and obvious how to use it.

Copy link
Member

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Thank you!

tika/tika.go Outdated
@@ -185,35 +197,35 @@ func (c *Client) ParseRecursive(ctx context.Context, input io.Reader) ([]string,

// Meta parses the metadata from the given input, returning the metadata and an
// error. If the error is not nil, the metadata is undefined.
func (c *Client) Meta(ctx context.Context, input io.Reader) (string, error) {
return c.callString(ctx, input, "PUT", "/meta")
func (c *Client) Meta(ctx context.Context, input io.Reader, header http.Header) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this still has the new header arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure. I'll fix it up. The issue with making the changes before breakfast this morning 😂

This applies the same changes as have been made to Parse.

I've also added a couple of extra tests to ensure the header is sent over correctly.

Apologies for missing this before.
@nathj07
Copy link
Contributor Author

nathj07 commented Apr 18, 2022

Apologies for missing the Meta function changes before. I've updated these functions now, the same way I have the others.

Conflicts:
    .gitignore

This conflict has been resolved.
Copy link
Member

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

No worries! Thanks for updating it. Would you mind running gofmt -w and go mod tidy? Looks like the lint check is failing.

Also ran go fmt, my editor had already run goimports so that yielded no changes.

go mod did update the mod files.
@nathj07
Copy link
Contributor Author

nathj07 commented Apr 18, 2022

Looks like go mod tidy was needed. I've pushed that now. Thanks

@tbpg tbpg merged commit 800d89e into google:main Apr 18, 2022
@nathj07 nathj07 deleted the issue-36-support-req-header-in-parse branch April 19, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants