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

Retain 'ua' when user specified wherever possible. #17

Merged
merged 8 commits into from Aug 14, 2014

Conversation

kentfredric
Copy link
Contributor

In response to rt#95796

By default, the value of 'ua' should be generated by MetaCPAN::Client,
and used **only** by MetaCPAN::Client, and Search::Elasticsearch should
be left to produce its own one.

However, if the user specifies a 'ua', then that 'ua' should be used for
both MetaCPAN::Client and Search::Elasticsearch.

Allowing users to specify their own 'ua' has risks of breaking things,
such as not injecting the right headers by default, so it is users
responsibility to get that part right.

Likewise, when we're using Search::Elasticsearch without needing any
'ua' customization, we should let it create its own one, which is more
likely to be "correct"

Implementation choices:

  previously; ->new( ua => )  mapped to ua =
      and ->ua returns that value verbatim.

  now: ->new( ua =>  ) maps to _user_ua
      and ->ua still returns that value verbatim.

However, the fact '_user_ua' is merely a predicate means we can later
do:

  my %cfg = (
       ...
  );
  if ( $self->_user_ua ) {
    cfg{handle} = $self->_user_ua;
  }
  Search::Elasticsearch->new( %cfg );

Wheras that wouldn't work with a pure 'ua', because 'has_ua' would
return true as soon as it had ever been lazy-built.
This allows clients to create objects that remember who created them,
and thus allow created objects to call subsequent requests sharing the
Clients configuration options. ( Especially in regard to UA )

If no such attribute is passed, the it will be created on demand when
needed.

This will make existing code continue to function even if it doesn't use
the MetaCPAN Client base directly.
…ling HTTP::Tiny directly

This presently fails on the existing MetaCPAN::Client but is fixed in
the pull request containing this test.
@oalders
Copy link
Member

oalders commented Aug 14, 2014

👍

@mickeyn
Copy link
Contributor

mickeyn commented Aug 14, 2014

@kentfredric thanks 👍

I'll look into it in more details and merge/release hopefully today.

mickeyn added a commit that referenced this pull request Aug 14, 2014
Retain 'ua' when user specified wherever possible.
@mickeyn mickeyn merged commit c0ea928 into metacpan:master Aug 14, 2014
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.

None yet

3 participants