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

gh-102542 Refactor the mime audio module #102540

Closed
wants to merge 9 commits into from

Conversation

JosephSBoyle
Copy link
Contributor

@JosephSBoyle JosephSBoyle commented Mar 8, 2023

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.

@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
@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
_subtype = _what(_audiodata)
if _subtype is None:
raise TypeError('Could not find audio MIME subtype')
_subtype = _subtype or _infer_subtype(_audiodata)
Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Member

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.

@terryjreedy
Copy link
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
Copy link
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.

@JosephSBoyle JosephSBoyle force-pushed the refactor-mime-audio branch 2 times, most recently from e3f62eb to 09cd7f1 Compare March 9, 2023 15:06
@JosephSBoyle
Copy link
Contributor Author

Force pushed to correct a mistake in the commit message.

@AlexWaygood
Copy link
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 :)

@JosephSBoyle JosephSBoyle force-pushed the refactor-mime-audio branch from ac8e652 to 5b26332 Compare March 9, 2023 19:56
@JosephSBoyle
Copy link
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

@JosephSBoyle JosephSBoyle requested a review from terryjreedy March 9, 2023 19:58
@JosephSBoyle
Copy link
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.

@JosephSBoyle JosephSBoyle changed the title gh-102542 TODO Refactor the mime audio module gh-102542 Refactor the mime audio module Apr 10, 2023
@JosephSBoyle
Copy link
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.

Copy link
Member

@terryjreedy terryjreedy left a 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.

@JosephSBoyle
Copy link
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

@warsaw
Copy link
Member

warsaw commented Jul 3, 2023

I've provided some additional thoughts on #102542

@JosephSBoyle
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants