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

Add the query_url method #47

Merged
merged 1 commit into from
Aug 1, 2014
Merged

Add the query_url method #47

merged 1 commit into from
Aug 1, 2014

Conversation

EdgarOrtegaRamirez
Copy link
Contributor

Closes #38 (Expose a method that turns parameters into a query URL)

Overview

Expose a method that turns parameters into a query URL, so the URL could be used to query asynchronously or from another HTTP library or system.

Solution

Add a query_url method that generates a URL for a query without running it, and allow the query method to be a consumer of this one.

Keen.query_url('users', {analysis_type: 'count'})
# https://api.keen.io/3.0/projects/12345/queries/count?event_collection=users

@joshed-io
Copy link
Contributor

Very cool!

Here are a few requests to take care of before we merge this in:

  • Can we make it so query is a consumer of query_url? It feels like that's the right pattern - query_url should handle making the string and query should just be doing the HTTP work.
  • Can we avoid using any instance or self.* variables? The client is designed to be stateless and so any instance variables could bleed between query calls.
  • Can you remove any of the whitespace and other refactoring diffs from the PR? It would be awesome to have those, but for the sake of readability and avoiding regressions I'd like to process them separately.
  • Can you add tests for missing project id / api key in query_url?

Really appreciate the PR, I think this'll be a popular feature!

This exposes a method that return the URL for a query without running it
@EdgarOrtegaRamirez
Copy link
Contributor Author

@dzello I just updated the PR, please take a look.

@joshed-io
Copy link
Contributor

Looks great, thanks!

joshed-io pushed a commit that referenced this pull request Aug 1, 2014
@joshed-io joshed-io merged commit 713bffa into keenlabs:master Aug 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Expose a method that turns parameters into a query URL
2 participants