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

V0.12.0 #175

Merged
47 commits merged into from Jan 3, 2019
Merged

V0.12.0 #175

47 commits merged into from Jan 3, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 20, 2018

Client

  • Introduced CogniteClient. Class with all api call methods made available through its instantiation. Ensures thread-safety. All sub clients (specific to APIs, e.g. files, events, timeseries, datapoints) inherit from APIClient class which give them access to sending requests through self.
  • Expose http verbs with plumbing and error handling taken care of through CogniteClient to make it easier for users to hit endpoints which are not covered by the sdk.
  • API Version is no longer exposed directly to user. 0.5 is default and some 0.6 features are exposed through an experimental sub client.
  • Changed CogniteDataObject to CogniteResponse and added a string representation for pretty printing responses.
  • Removed .to_ndarray() support for response objects
  • Logging now defaults to log to DEBUG but log_level can be configured through CogniteClient object.

Restructuring

  • All configuration is moved from config module to CogniteClient
  • Everything requests related is moved to APIClient
  • Preprocessing module removed
  • Removed everything related to CLI which was abandoned
  • _constants module removed
  • config module removed

Documentation

  • Improved overall documentation
  • Added some simple examples for all queries.

Tests

  • Tests no longer dependent on time series stored in mltest tenant

Data Transfer Service

  • Updated to use new client interface

@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@codecov-io
Copy link

codecov-io commented Dec 23, 2018

Codecov Report

Merging #175 into master will decrease coverage by 3.71%.
The diff coverage is 86.99%.

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   91.38%   87.66%   -3.72%     
==========================================
  Files          29       19      -10     
  Lines        2344     1435     -909     
==========================================
- Hits         2142     1258     -884     
+ Misses        202      177      -25
Impacted Files Coverage Δ
cognite/client/_utils.py 100% <100%> (ø)
cognite/client/stable/login.py 100% <100%> (ø)
cognite/client/experimental/analytics/__init__.py 100% <100%> (ø)
cognite/client/experimental/__init__.py 100% <100%> (ø)
cognite/client/exceptions.py 100% <100%> (ø)
cognite/client/experimental/analytics/models.py 50% <50%> (ø)
cognite/data_transfer_service.py 92.18% <50%> (+0.18%) ⬆️
cognite/client/experimental/time_series.py 65.62% <65.62%> (ø)
cognite/client/experimental/sequences.py 66.1% <70.45%> (ø)
cognite/client/stable/assets.py 85.13% <85.13%> (ø)
... and 26 more

@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@cognitedata cognitedata deleted a comment from cognite-cicd Dec 23, 2018
@ghost
Copy link
Author

ghost commented Jan 2, 2019

Maybe since there have been such substantial changes to the interface, this should be a major, not a minor, version bump? @NilsBarlaug-cognite what do you think?

@ghost ghost requested a review from NilsBarlaug-cognite January 2, 2019 20:18
@cognite-cicd

This comment has been minimized.

@cognitedata cognitedata deleted a comment from cognite-cicd Jan 2, 2019
@cognitedata cognitedata deleted a comment from cognite-cicd Jan 2, 2019
@cognitedata cognitedata deleted a comment from cognite-cicd Jan 2, 2019
@NilsBarlaug-cognite
Copy link
Contributor

Maybe since there have been such substantial changes to the interface, this should be a major, not a minor, version bump? @NilsBarlaug-cognite what do you think?

I guess we should normally do that, but since we're in major version zero it depends.

  1. "Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable."
  2. "Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes."

(see https://semver.org)

So if we bump to version 1 that would mean we have officially released the first stable version and are not in initial development anymore. I would argue that it doesn't make sense to be major version 1 before the actual underlying API is stable (version 1), since we then can't guarantee that the SDK will not break. In addition, since there are so big changes now it will probably be followed by more changes and bug fixes the coming weeks. We should at least be somewhat stable before we go to major version 1.

What do you think @f1cognite?

Copy link
Contributor

@NilsBarlaug-cognite NilsBarlaug-cognite left a comment

Choose a reason for hiding this comment

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

⭐️⭐️⭐️⭐️⭐️

@ghost ghost merged commit e3c241c into master Jan 3, 2019
@ghost ghost deleted the new-sdk branch January 3, 2019 10:09
alchemistake pushed a commit that referenced this pull request Jun 23, 2022
* Major rewriting of SDK

* Improve documentation

* Update docs and make api client limits protected

* Change from numbered versioning to stable/experimental

* Remove unused code and fix __str__() for CogniteResponse

* Remove anything related to versions from documentation

* Minor fixes and updates to docstrings

* Update api key env var in Jenkinsfile

* Update CONTRIBUTING.md

* Remove unused print statements and add debug mode

* Omit auxiliary directory from coverage report

* Increase coverage threshold to 2%

* Update documentation and docstrings

* Add examples in docs for assets and datapoints

* Fix assets test

* Add simple examples for events, files, login and time_series

* Fix examples in overview

* Skip undeterministically failing sequences test

* Update main docs

* Increase coverage threshold

* Fix typo in docs

* Update jupyter examples

* Clean up docs and fix ordering issue

* Move version specification in to each individual client

* Make requested changes from F1's review

* Update documentation

* Remove docstring from top-level __init__.py

* Do not retry 401 or POST

* Add default timeout of 60s for all requests and let user configure it.

* Fix bug in response object __str__()

* Return None instead of empty dict where applicable

* Remove some outdated kwargs from docstrings

* Do not return empty dict as string repr for cognite response when 'data' key is missing

* Move to_json() method to CogniteResponse and remove where it is not necessary to override

* Use .to_json() in .__str__() of CogniteResponse

* Fix failing tests after fixes

* Move helpers only used in tests to conftest.py

* Update some docstrings and include inherited members in docs

* Add more examples

* Fix bug in get_datapoints multithreading

* Fix bug in example

* Remove version parameter in client_factory and make it protected.

* Update CONTRIBUTING.md with link to semver.org instead of this homemade versioning scheme

* Default to logging at INFO level and do not let user configure this

* Fix bug in retry logic causing client to wait after last retry
This pull request was closed.
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

4 participants