Artifacts #12
Artifacts #12
Conversation
|
Wait for review to upgrade package version |
tests/test_artifacts.py
Outdated
| artifactory = ArtifactoryArtifact(AuthModel(url=URL, auth=AUTH)) | ||
| artifact = artifactory.download(ARTIFACT_PATH) | ||
| os.remove("file.txt") |
There was a problem hiding this comment.
You can use the "Temporary files & directories" fixtures from Pytest to avoid deleting files: https://docs.pytest.org/en/latest/tmpdir.html
tests/test_artifacts.py
Outdated
| @@ -0,0 +1,123 @@ | |||
| import os | |||
There was a problem hiding this comment.
Do we check any failure case in this test?
There was a problem hiding this comment.
It's only used for removing the file.txt file
There was a problem hiding this comment.
I wasn't talking about the os import, I just wanted to add a comment to the whole file... But anyway, you told me that Artifactory cannot fail, so there is no test for that.
Maybe at least something like testing a download for which the artifact is not found?
docs: add black badge
README.md
Outdated
|
|
||
| # Update user | ||
| user.email = "[email protected]" | ||
| updated_user = art.users.update(User(**user.dict())) |
There was a problem hiding this comment.
With your version of Pydantic you have access to the User.parse_obj method, which is exactly for this use case: https://pydantic-docs.helpmanual.io/usage/models/#helper-functions
There was a problem hiding this comment.
Also, why do you need to create a new user from the old instance? Can't you just pass the modified user?
There was a problem hiding this comment.
Right, I can pass the pass the modified user
| class User(BaseUserModel): | ||
| email: Optional[EmailStr] = None | ||
|
|
||
|
|
There was a problem hiding this comment.
What does this new model represent? (Just asking ^^)
There was a problem hiding this comment.
This is for fixing the update user issue. Indeed, the method update user used to require a NewUser object. However, this object contains a password, which is problematic for update operations. Therefore, I created this new model only for the update operation.
pyartifactory/objects.py
Outdated
| if local_file_path: | ||
| Path(local_file_path).mkdir(parents=True, exist_ok=True) | ||
| local_file_full_path = f"{local_file_path}/{local_filename}" |
There was a problem hiding this comment.
If you call your method parameter local_file_path, I expect the artifact to be saved under this exact name, and not to create a directory called local_file_path in which the artifact will be saved.
| requests = "^2.21" | ||
| email_validator = "^1.0" | ||
| requests_toolbelt = "^0.9.1" | ||
| pydantic = "^1.3" | ||
|
|
There was a problem hiding this comment.
(Adding a comment on this line because I cannot add it below)
If you upgrade the versions of mypy, black and detect-secrets in the pre-commit config, you should update them here too ^^
tests/test_artifacts.py
Outdated
| ) | ||
|
|
||
| artifactory = ArtifactoryArtifact(AuthModel(url=URL, auth=AUTH)) | ||
| artifact = artifactory.download(ARTIFACT_PATH, LOCAL_FILE_PATH) |
There was a problem hiding this comment.
I would really use the tmp_path Pytest fixture instead of managing test created files manually...
tests/test_artifacts.py
Outdated
| artifactory = ArtifactoryArtifact(AuthModel(url=URL, auth=AUTH)) | ||
| artifact = artifactory.download(ARTIFACT_PATH, LOCAL_FILE_PATH) | ||
|
|
||
| assert artifact == f"{LOCAL_FILE_PATH}/{artifact_name}" |
There was a problem hiding this comment.
Could you also assert that a file has really been created?
|
Complexity increasing per file
==============================
- tests/test_artifacts.py 3
- pyartifactory/objects.py 2
- pyartifactory/models/Artifact.py 1
Clones removed
==============
+ tests/test_users.py -2
See the complete overview on Codacy |
No description provided.