Skip to content

Release threshold folder imports nit #56400

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Sep 19, 2023
Merged

Release threshold folder imports nit #56400

merged 8 commits into from
Sep 19, 2023

Conversation

nhsiehgit
Copy link
Contributor

Cleans up release threshold model imports & adds related name to the relase_threshold table for projects

@nhsiehgit nhsiehgit requested a review from a team as a code owner September 18, 2023 17:25
@nhsiehgit nhsiehgit requested review from cathteng and a team September 18, 2023 17:25
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 18, 2023
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0555_threshold_related_name.py ()

--
-- Alter field project on releasethreshold
--

cathteng
cathteng reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@cathteng cathteng left a comment

Choose a reason for hiding this comment

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

lgtm, but maybe get confirmation from someone from migrations?

src/sentry/api/endpoints/release_threshold_details.py
@@ -10,8 +10,7 @@
from sentry.api.bases.project import ProjectEndpoint
from sentry.api.exceptions import ResourceDoesNotExist
from sentry.api.serializers import serialize
from sentry.models.project import Project
from sentry.models.release_threshold.releasethreshold import ReleaseThreshold
Copy link
Member

Choose a reason for hiding this comment

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

where do we prefer importing models like this? i see both styles sometimes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specific imports are fine, however we're already exporting everything from the models/init.py so imo it's cleaner to simply have the top level path for imports.

Copy link
Member

Choose a reason for hiding this comment

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

I think we're trying to get away from this like @JoshFerge pointed out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - sounds like we're actually pushing for the explicit imports so reverting and changing to that

@codecov
Copy link

codecov bot commented Sep 18, 2023 •
edited
Loading

Codecov Report

Merging #56400 (7cb4d6f) into master (ee94913) will decrease coverage by 0.01%.
Report is 21 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 7cb4d6f differs from pull request most recent head 0794eb4. Consider uploading reports for the commit 0794eb4 to get more accurate results

@@            Coverage Diff             @@
##           master   #56400      +/-   ##
==========================================
- Coverage   78.63%   78.62%   -0.01%     
==========================================
  Files        5077     5078       +1     
  Lines      218315   218349      +34     
  Branches    36951    36961      +10     
==========================================
+ Hits       171671   171687      +16     
- Misses      41100    41114      +14     
- Partials     5544     5548       +4
Files Changed Coverage
src/sentry/models/__init__.py ø
...ntry/models/release_threshold/release_threshold.py ø
src/sentry/api/endpoints/release_threshold.py 100.00%
.../sentry/api/endpoints/release_threshold_details.py 100.00%
...sentry/api/serializers/models/release_threshold.py 100.00%
src/sentry/models/release_threshold/constants.py 100.00%

JoshFerge
JoshFerge reviewed Sep 18, 2023
View reviewed changes
src/sentry/api/serializers/models/release_threshold.py Outdated
@@ -1,9 +1,9 @@
from sentry.api.serializers import Serializer, register, serialize
from sentry.models import ReleaseThreshold
Copy link
Member

Choose a reason for hiding this comment

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

should favor importing directly from the model file. (i.e. import from sentry.models.releasethreshold). We should work towards enforcing this somewhere

@nhsiehgit nhsiehgit force-pushed the release_threshold_nit_migration branch from 0b4f99d to 666054a Compare September 18, 2023 21:14
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0556_threshold_related_name.py ()

--
-- Alter field project on releasethreshold
--

JoshFerge
JoshFerge reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

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

could we isolate the migration from the nits? try in general to keep migration PRs separate from other changes.

@nhsiehgit nhsiehgit mentioned this pull request Sep 18, 2023
Merged
@nhsiehgit nhsiehgit changed the title Release threshold nit migration Release threshold folder imports nit Sep 18, 2023
nhsiehgit added a commit that referenced this pull request Sep 18, 2023
@nhsiehgit
release trheshold related name migration (#56429)
dcf999a 
Separate Migration from nits in
#56400
@nhsiehgit nhsiehgit force-pushed the release_threshold_nit_migration branch from 8175ac5 to 79d8972 Compare September 18, 2023 22:42
@nhsiehgit nhsiehgit requested a review from JoshFerge September 18, 2023 22:49
@nhsiehgit nhsiehgit enabled auto-merge (squash) September 18, 2023 22:50
schew2381 pushed a commit that referenced this pull request Sep 19, 2023
@nhsiehgit @schew2381
release trheshold related name migration (#56429)
6d02a82 
Separate Migration from nits in
#56400
@nhsiehgit nhsiehgit merged commit 17d7b08 into master Sep 19, 2023
@nhsiehgit nhsiehgit deleted the release_threshold_nit_migration branch September 19, 2023 16:25
@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@evanpurkhiser evanpurkhiser evanpurkhiser left review comments

@JoshFerge JoshFerge JoshFerge approved these changes

@cathteng cathteng Awaiting requested review from cathteng

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nhsiehgit @evanpurkhiser @JoshFerge @cathteng