Skip to content

fix: subject_type for "foo:bar" event tags#19998

Open
divbzero wants to merge 1 commit into
pypi:mainfrom
divbzero:event-tag-omitted-subject-type
Open

fix: subject_type for "foo:bar" event tags#19998
divbzero wants to merge 1 commit into
pypi:mainfrom
divbzero:event-tag-omitted-subject-type

Conversation

@divbzero
Copy link
Copy Markdown
Contributor

@divbzero divbzero commented May 2, 2026

The EventTagEnum docstring indicates that omitted subject_type should fallback to source_type:

If omitted, subject type is implied to be the same as source type.

However, a typo resulted in EventTagEnum("foo:bar") to have "f" instead of "foo" for subject_type.

@divbzero divbzero requested a review from a team as a code owner May 2, 2026 05:02
@divbzero divbzero changed the title Fix EventTagEnum.subject_type for "foo:bar" tags Fix EventTagEnum.subject_type for "foo:bar" tags May 2, 2026
@divbzero divbzero changed the title Fix EventTagEnum.subject_type for "foo:bar" tags Fix subject_type for "foo:bar" event tags May 2, 2026
@divbzero divbzero changed the title Fix subject_type for "foo:bar" event tags fix: subject_type for "foo:bar" event tags May 2, 2026
Copy link
Copy Markdown
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

How does this manifest? Feels odd to see a fix like this with no failing or updated tests?

@divbzero
Copy link
Copy Markdown
Contributor Author

divbzero commented May 5, 2026

@miketheman I don’t think it actually manifests. subject_type is defined for EventTagEnum but isn’t used anywhere in the code base.

I contributed to pypi/warehouse awhile back and was reading the code again as a reference for a different project I’m working on. Just happened to spot this typo.

Does the fix look sensible as is? Would we want to add tests, or perhaps remove source_type, subject_type, and action from __new__ if they aren’t used anywhere?

@miketheman
Copy link
Copy Markdown
Member

Does the fix look sensible as is? Would we want to add tests, or perhaps remove source_type, subject_type, and action from __new__ if they aren’t used anywhere?

I'm thinking that if we don't actually make the distinction anywhere, and use it as the raw string, the parsing/assignment could be removed? But on the off chance we do use it in some weird way, it's probably safer to keep it, so adding tests is a better approach - maybe the tests would show the intended usage.

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