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

Implemented the query2 API #35

Merged
merged 9 commits into from Jan 22, 2018

Conversation

Projects
None yet
2 participants
@johan-bjareholt
Copy link
Member

johan-bjareholt commented Oct 7, 2017

  • Removed ChunkedEvents resource
  • Removed View resources
  • Added Query resource based on query2 with built-in caching
@johan-bjareholt

This comment has been minimized.

Copy link
Member

johan-bjareholt commented Oct 7, 2017

Build fails because aw-core is not updated

@wafflebot wafflebot bot added review and removed in progress labels Oct 14, 2017


result = app.api.query_view(viewname, start, end)
# It's not possible to expect a raw string in restplus/swagger, stupid

This comment has been minimized.

@ErikBjare

ErikBjare Oct 14, 2017

Member

I think the way to do this is to simply create a query model that contains only one field. Something like this:

query_fields = api.model('Query', {
    'query': fields.String,
})

Is there any reason this is not an option?

@johan-bjareholt

This comment has been minimized.

Copy link
Member

johan-bjareholt commented Oct 15, 2017

@ErikBjare

This comment has been minimized.

Copy link
Member

ErikBjare commented Oct 15, 2017

Ah, alright. In that case, we might want to consider using semicolons instead of newlines in the syntax. Kind of makes more sense in a DSL like this in my opinion (less fiddling with newlines).

I'd still prefer to have the JSON query model there. A clean REST API using format X (in this case JSON) should rarely - if ever - have the payloads delivered in a not-X format. I myself don't mind wrapping the DSL code in a JSON object.

Just a thought, I might be wrong about this: Perhaps some parts of the DSL language (such as the start/end dates) would be better specified as arguments instead of as a part of the DSL? Would clean up the DSL code construction before making a query.

Regardless I think it's nice to have the JSON wrapper for the sake of consistency. Any objections?

@johan-bjareholt

This comment has been minimized.

Copy link
Member

johan-bjareholt commented Oct 15, 2017

It's just so incredibly clunky to do multi-line JSON strings, so to write it in in a readable manner in python was a hassle. Regarding starttime, endtime and name I agree though. Can look into it again, but since JSON technically does not support multi-line strings we will have to do workarounds no matter what language it is, so it might get very ugly, but as I said I'll look into it. Readability counts.

@ErikBjare

This comment has been minimized.

Copy link
Member

ErikBjare commented Oct 31, 2017

@johan-bjareholt In the future, please don't remove code as soon as a replacement is implemented. Instead, comment or otherwise mark it as deprecated.

That way we won't have to ensure that all dependents on the deprecated code are migrated right away. If you, for example, would have kept the old views endpoint here then we could have merged this a lot earlier because we wouldn't have to migrate all the code using views to query2 before merging.

I should probably add this to the docs.

Edit: Rich Hickey (Clojure creator) has a talk (that I'm unable to find) where he talks about writing code that never breaks in updates (need a non-compatible version of function_x? Create a new one called function_x2 or make a new package2). While I do intent to deprecate code and eventually remove it, we should deprecate it slowly and not remove it all-at-once.

@ErikBjare ErikBjare referenced this pull request Oct 31, 2017

Merged

WIP: Dev/transform-next aka query2 #46

11 of 12 tasks complete

@johan-bjareholt johan-bjareholt force-pushed the dev/transform-next branch from 87d3d32 to 05bddf4 Nov 11, 2017

@johan-bjareholt johan-bjareholt force-pushed the dev/transform-next branch from 8c56d68 to babeb60 Jan 21, 2018

@ErikBjare ErikBjare merged commit 076afaa into master Jan 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the review label Jan 22, 2018

@ErikBjare ErikBjare deleted the dev/transform-next branch Jan 22, 2018

@ErikBjare ErikBjare changed the title WIP: transform-next Implemented the query2 API May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment