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

Begin DCP Query Service CLI #359

Merged
merged 2 commits into from
Jun 13, 2019
Merged

Begin DCP Query Service CLI #359

merged 2 commits into from
Jun 13, 2019

Conversation

kislyuk
Copy link
Member

@kislyuk kislyuk commented Jun 13, 2019

Because the DCP Query Service uses Swagger 3.0/OpenAPI 3.0, this also begins OpenAPI 3.0 compatibility work in the Swagger Client.

Closes HumanCellAtlas/query-service#185

Because the DCP Query Service uses Swagger 3.0/OpenAPI 3.0, this also
begins OpenAPI 3.0 compatibility work in the Swagger Client.
@kislyuk kislyuk requested review from MDunitz and DailyDreaming June 13, 2019 16:27
Copy link
Contributor

@DailyDreaming DailyDreaming left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of comments.

def help(args):
query_parser.print_help()

if sys.version_info >= (2, 7, 9): # See https://bugs.python.org/issue9351
Copy link
Contributor

@DailyDreaming DailyDreaming Jun 13, 2019

Choose a reason for hiding this comment

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

Do we need to worry about python < 2.7.9? This surprised me.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #112

self.host = "{scheme}://{host}{base}".format(scheme=self.scheme,
host=self.swagger_spec["host"],
base=self.swagger_spec["basePath"])
if "openapi" in self.swagger_spec:
Copy link
Contributor

@DailyDreaming DailyDreaming Jun 13, 2019

Choose a reason for hiding this comment

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

Nit: Would it make sense to have a variable similar to USING_PYTHON2 specifying the swagger version (within the client)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think hardcoding the version of the API spec language in each client is necessary/good, because it makes upgrading the server-side version harder. Instead the client should ideally detect the version of the server-supplied spec and adjust accordingly (which is what is done here).

I do agree that version-specific behavior could be better separated in this class. In the interest of expediency I just put a few if statements here, but ideally we would separate the code more thoroughly or even factor it out into an "OpenAPISpecParser" and "SwaggerAPISpecParser" or something.

@codecov-io
Copy link

codecov-io commented Jun 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@4a82d48). Click here to learn what that means.
The diff coverage is 97.82%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #359   +/-   ##
=========================================
  Coverage          ?   84.98%           
=========================================
  Files             ?       39           
  Lines             ?     1785           
  Branches          ?        0           
=========================================
  Hits              ?     1517           
  Misses            ?      268           
  Partials          ?        0
Impacted Files Coverage Δ
hca/cli.py 59.48% <100%> (ø)
hca/util/__init__.py 86.36% <100%> (ø)
hca/query/__init__.py 100% <100%> (ø)
hca/query/cli.py 91.66% <91.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a82d48...8408ac8. Read the comment docs.

@kislyuk
Copy link
Member Author

kislyuk commented Jun 13, 2019

Ignoring codeclimate report. Also, there is an issue with the coverage reporting for this build. Not sure what happened.

@kislyuk kislyuk merged commit 33e694a into master Jun 13, 2019
@kislyuk kislyuk deleted the akislyuk-begin-dcpquery-cli branch June 13, 2019 20:36
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.

Add a Query Service client to the DCP CLI
3 participants