fixed trunk issues in conftest#44
Conversation
|
Hey @abdurrehman11 thanks for colaborate! |
| "password": "pass_mock", | ||
| } | ||
| response = db_mocked_app_client.post(url="user/register", json=user_info) | ||
| assert 201 == response.status_code |
There was a problem hiding this comment.
This change doesn't change the behavior of the code.
The assert statement does exactly the same as you made - check it: https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement
There was a problem hiding this comment.
sure, I have drafted this PR.
This change was made to fix trunk issues so it should not do any change in the behaviour of the code as discussed here: #34
But if we want to keep assert as it is currently being used, we can configure the trunk to ignore assert statements. What do you say?
There was a problem hiding this comment.
also, do we really to fix all trunk issues with current trunk default configuration as it require a lot of changes to do in the code, For example, if I fix trunk issue in app/core/db/migrations/models.py, then I have to remove the below both imports in this file as they are not being used anywhere
from app.core.models.record import Record
from app.services.user.models import RevokedToken, User
trunk raise issues in this file are the following,
app/core/db/migrations/models.py:1:1
1:1 high 'app.core.models.record.Record' imported but unused flake8/F401
1:36 high `app.core.models.record.Record` imported but unused ruff/F401
2:1 high 'app.services.user.models.User' imported but unused flake8/F401
2:1 high 'app.services.user.models.RevokedToken' imported but unused flake8/F401
2:38 high `app.services.user.models.RevokedToken` imported but unused ruff/F401
2:52 high `app.services.user.models.User` imported but unused ruff/F401
Please let me know your thoughts on how we want to go for these issues fix? fix all as per truck default recommendations or we need to modify trunk?
There was a problem hiding this comment.
wow, I got it now, sorry!! so, never mind what I said
I'm in trouble with trunk 😭
There was a problem hiding this comment.
my guess is, that we definitely need to fix most of them. but of course, some of them don't make any sense to be fixed, so we can ignore using the #noqa notation
There was a problem hiding this comment.
ok, will try to resolve as much as possible and then submit PR for review
There was a problem hiding this comment.
In app/core/admin/models.py, on line 56: model: object = await self.get_object_for_edit(pk), can we replace the model type from object to ModelCore from app/core/models/base.py
This is a high severity issue in the trunk.
I am confused because I don't fully understand the codebase at this point but I am trying to understand it gradually.
There was a problem hiding this comment.
@pedroimpulcetto I commit the trunk issues fix, looking forward to review.
2 participants
Hi,
I just fixed trunk issues in one file and created PR for review to make sure I am going in the right direction and open to any feedback. Please don't merge this PR into main right now, later I will fix all other issues and commit them in the same PR and then we can move for merge after review.
Thanks,