Extract apiclient from dmutils #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Audit event types enum is used only for the API client calls, so it makes sense to group it with the API client module.
Tests for request headers were using the `dmutils.request_id` module to set up the Request class with `.request_id` attribute. Since the test is supposed to check that API client is setting the header on the API request, we can configure the Flask request object directly instead of relying on `request_id.init_app`.
Using setuptools namespace_packages to keep the existing apiclient import paths. (https://pythonhosted.org/setuptools/setuptools.html#namespace-packages) This makes any code in `dmutils/__init__.py` inaccessible from the installed package, so we need to move the version tag to `dmutils.apiclient`. Having a constant declaration above module-level imports triggers a pep8 warning without `noqa`, even though `__version__` should be declared after the module docstring (https://www.python.org/dev/peps/pep-0008/#version-bookkeeping).
Using dmutils namespace with namespace_packages requires removing all code from `dmutils.__init__`, which is currently acting as an import proxy for third-party packages. Removing `__init__.py` will break app imports, which is the main reason for package namespacing. Instead, we create a new package name `dmapiclient`, which allows us to simplify the directory structure by removing the nested `apiclient` package. The existing `dmutils.apiclient` import path can be preserved by adding `import dmapiclient as apiclient` to `dmutils/__init__.py`.
|
|
||
| ## What's in here? | ||
|
|
||
| Digital Marketplace Data and Search API clients |
There was a problem hiding this comment.
This could do with a bit more info. Maybe just an example and links to the other relevant repos.
|
Other than the README point this looks ace 👍 🙏 (that's meant to be a high five) |
allait
force-pushed
the
extract-apiclient-from-dmutils
branch
from
90584fb
to
c56a169
Compare
|
Expanded the README a bit and added |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Copy apiclient modules and tests from digitalmarketplace-utils
(https://github.com/alphagov/digitalmarketplace-utils/tree/63531bd4d2c1bed8774fef04aaf3ac8f71d341db)
Add dmutils.audit and audit types tests from digitalmarketplace-utils
Audit event types enum is used only for the API client calls, so it
makes sense to group it with the API client module.
Rewrite apiclient tests to avoid dmutils.request_id dependency
Tests for request headers were using the
dmutils.request_idmoduleto set up the Request class with
.request_idattribute. Since thetest is supposed to check that API client is setting the header on
the API request, we can configure the Flask request object directly
instead of relying on
request_id.init_app.Use dmapiclient package name instead of dmutils namespacing
Using dmutils namespace with namespace_packages requires removing
all code from
dmutils.__init__, which is currently acting as animport proxy for third-party packages. Removing
__init__.pywillbreak app imports, which is the main reason for package namespacing.
Instead, we create a new package name
dmapiclient, which allows usto simplify the directory structure by removing the nested
apiclientpackage. The existing
dmutils.apiclientimport path can be preservedby adding
import dmapiclient as apiclienttodmutils/__init__.py.