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

fixed trunk issues in conftest #44

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

abdur-n-tr
Copy link

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,

@pedroimpulcetto
Copy link

Hey @abdurrehman11 thanks for colaborate!
First of all, if your PR is not done to be merged, please keep it in Draft to avoid any issues.

@@ -77,7 +77,10 @@ def user_mocked_info(db_mocked_app_client):
"password": "pass_mock",
}
response = db_mocked_app_client.post(url="user/register", json=user_info)
assert 201 == response.status_code

Choose a reason for hiding this comment

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

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

Copy link
Author

@abdur-n-tr abdur-n-tr May 3, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

wow, I got it now, sorry!! so, never mind what I said

I'm in trouble with trunk 😭

Screenshot 2024-05-03 at 10 17 54 AM

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

ok, will try to resolve as much as possible and then submit PR for review

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@pedroimpulcetto I commit the trunk issues fix, looking forward to review.

@abdur-n-tr abdur-n-tr marked this pull request as draft May 3, 2024 09:55
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.

2 participants