GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-102542 Refactor the mime audio module by JosephSBoyle · Pull Request #102540 · python/cpython

Conversation

@JosephSBoyle

@JosephSBoyle JosephSBoyle commented Mar 8, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Refactor the MIME audio module, the mechanism for attempting to infer a subtype is currently quite verbose.

Notes:

  • fakefile is not used in any of the functions it is passed to, hence it has been removed.
  • Slicing the header (hrd = data[:512]) isn't necessary so has been removed.
  • Issue: Refactor the MIMEAudio module #102542

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting review label Mar 8, 2023
@JosephSBoyle
JosephSBoyle force-pushed the refactor-mime-audio branch from e60185c to 44c5f8c Compare March 8, 2023 21:59
@JosephSBoyle JosephSBoyle changed the title gh-XXXX TODO Refactor the mime audio module gh-102542 TODO Refactor the mime audio module Mar 8, 2023
@bedevere-bot bedevere-bot mentioned this pull request Mar 8, 2023
Refactor the MIMEAudio module #102542
Closed
@JosephSBoyle
JosephSBoyle marked this pull request as ready for review March 8, 2023 22:02
@JosephSBoyle
JosephSBoyle requested a review from a team as a code owner March 8, 2023 22:02
terryjreedy
terryjreedy reviewed Mar 8, 2023
View reviewed changes
Comment thread Lib/email/mime/audio.py
_subtype = _what(_audiodata)
if _subtype is None:
raise TypeError('Could not find audio MIME subtype')
_subtype = _subtype or _infer_subtype(_audiodata)

@terryjreedy terryjreedy Mar 8, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This changes the behavior if _subtype is '' (or any other non-None null value). Someone other than me needs to agree with this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JosephSBoyle JosephSBoyle Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That's correct. Falsey values other than None such as: 0, False, '' and empty collections will all fall back to the second or operand: _infer_subtype(_audiodata).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@warsaw warsaw Jul 3, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I personally prefer more explicit tests than just "falseiness" when a more explicit type is indicated. In this case _subtype=None defaults to None to mean that this argument wasn't given.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@terryjreedy

terryjreedy commented Mar 8, 2023

Copy link
Copy Markdown
Member

@warsaw This PR mostly replaces the section you committed April '22. I believe that replacement logic is correct.

Joseph, have you checked the coverage of the mime audio test?
JosephSBoyle reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JosephSBoyle

JosephSBoyle commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

@warsaw This PR mostly replaces the section you committed April '22. I believe that replacement logic is correct.

Joseph, have you checked the coverage of the mime audio test?

None of the tests execute this path as far as I could tell from putting a breakpoint in and running the tests.

I convinced myself that the logic in _infer_subtype was correct by just running the script in a REPL and evaluating the output of different input strings.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JosephSBoyle
JosephSBoyle force-pushed the refactor-mime-audio branch 2 times, most recently from e3f62eb to 09cd7f1 Compare March 9, 2023 15:06
@JosephSBoyle

JosephSBoyle commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Force pushed to correct a mistake in the commit message.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Mar 9, 2023

Copy link
Copy Markdown
Member

None of the tests execute this path as far as I could tell from putting a breakpoint in and running the tests.

That implies that we should perhaps first work on improving test coverage, before undertaking a risky refactoring of this module.

("Risky" only in the sense that all refactorings are risky if test coverage is low :)
warsaw reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JosephSBoyle added 4 commits March 9, 2023 19:56
@JosephSBoyle
Refactor the mime audio module
496bbec
@JosephSBoyle
Improve comparisons in _infer_subtype
3561ee9
@JosephSBoyle
Add a test for inferring the audio content subtypes
15cd8ad
@JosephSBoyle
Make the infer audio content subtypes test more robust
5b26332
@JosephSBoyle
JosephSBoyle force-pushed the refactor-mime-audio branch from ac8e652 to 5b26332 Compare March 9, 2023 19:56
@JosephSBoyle

JosephSBoyle commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

That implies that we should perhaps first work on improving test coverage, before undertaking a risky refactoring of this module.

You're right as usual;) Turns out adding the tests was easier than I first thought.

Apologies for rebasing rather than merging, I'm still working on using that workflow @AlexWaygood
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JosephSBoyle
JosephSBoyle requested a review from terryjreedy March 9, 2023 19:58
@JosephSBoyle
Remove newly added redundant tests
1b4b061
@JosephSBoyle

JosephSBoyle commented Mar 11, 2023

Copy link
Copy Markdown
Contributor Author

Update: I was incorrect about this path not being tested - there are in fact tests for this, see: TestMIMEAudio::test_guess_minor_type.

As such I've deleted the redundant ones.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JosephSBoyle
Merge branch 'main' into refactor-mime-audio
3ebbfae
@arhadthedev arhadthedev added skip news topic-email labels Apr 1, 2023
@JosephSBoyle JosephSBoyle changed the title gh-102542 TODO Refactor the mime audio module gh-102542 Refactor the mime audio module Apr 10, 2023
@JosephSBoyle
Merge branch 'python:main' into refactor-mime-audio
114e324
@JosephSBoyle

JosephSBoyle commented May 23, 2023

Copy link
Copy Markdown
Contributor Author

@terryjreedy could you have a re-review when you get a chance please.

I think the minimum set of changes for this PR would be simply removing the unused fakefile that is passed to each function as this isn't used whatsoever. If the other changes don't seem worthwhile let me know and they can be reverted.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
terryjreedy
terryjreedy reviewed May 24, 2023
View reviewed changes

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I have nothing to add, which was that I will not approve the change as this is outside my expertise.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JosephSBoyle

JosephSBoyle commented May 24, 2023

Copy link
Copy Markdown
Contributor Author

Hia @AlexWaygood. Who I should request a review from on this? I have the feeling it may have slipped through people's todos!

Cheers
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JosephSBoyle added 2 commits June 12, 2023 17:33
@JosephSBoyle
Merge branch 'main' into refactor-mime-audio
1f1efa5
@JosephSBoyle
Merge branch 'main' into refactor-mime-audio
9d21be1
@warsaw

warsaw commented Jul 3, 2023

Copy link
Copy Markdown
Member

I've provided some additional thoughts on #102542
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JosephSBoyle

JosephSBoyle commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

Closing as we've decided to leave the @rule decorator for extensibility reasons; see the issue for discussion.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JosephSBoyle JosephSBoyle closed this Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@warsaw warsaw warsaw left review comments
@terryjreedy terryjreedy terryjreedy left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review skip news topic-email

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@JosephSBoyle @terryjreedy @AlexWaygood @warsaw @arhadthedev @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.