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

Refactor the MIMEAudio module #102542

Closed
JosephSBoyle opened this issue Mar 8, 2023 · 5 comments
Closed

Refactor the MIMEAudio module #102542

JosephSBoyle opened this issue Mar 8, 2023 · 5 comments
Labels
topic-email type-feature A feature request or enhancement

Comments

@JosephSBoyle
Copy link
Contributor

JosephSBoyle commented Mar 8, 2023

We can remove almost half of this file without losing any expressiveness and marginally improve performance at the same time.

Linked PRs

@warsaw
Copy link
Member

warsaw commented Jul 3, 2023

@JosephSBoyle - thanks for your contributions. I'm commenting here rather than on the PR (although I did add some feedback over there). Please also notice that Lib/email/mime/image.py uses a similar "rules" based mechanism to infer the image/* subtype when not provided.

One reason I went with this in my rewrite was so that we could easily extend the list of subtypes that we could recognize. Just add another @rule and you're done. While rule() isn't explicitly exposed publicly in either module, one could conceivably extend the list of subtypes in 3rd party code by doing something like:

from email.mime.audio import rule

@rule
def _my_image_type(h):
    if h[:5] == 'FLUFL':
        return 'flufl'

I wouldn't want to lose that implicit extensibility unless there's a good reason to. I'm not sure module size or "marginally improved performance" are compelling enough. Can you provide some performance numbers? Even if they were mind-blowingly better, I'm not sure this is performance critical code.

@JosephSBoyle
Copy link
Contributor Author

Hi @warsaw, thanks for taking a look at the PR.

I wouldn't want to lose that implicit extensibility unless there's a good reason to

Fair enough, I'm sure that someone is registering their own rules so it makes sense to keep that functionality 😄


Would you accept a new, smaller PR to remove the unnecessary header slicing and unused fakefile instance?

@warsaw
Copy link
Member

warsaw commented Jul 4, 2023

Would you accept a new, smaller PR to remove the unnecessary header slicing and unused fakefile instance?

I would! Yeah, there's no reason any more to slice the data and it makes sense for audio.py and image.py to have a similar implementation (but not the same implementation because they have different _rules).

@JosephSBoyle
Copy link
Contributor Author

Awesome, I've created the new PR here: #106433.

@warsaw

warsaw pushed a commit that referenced this issue Jul 5, 2023
Remove unused bytes object and bytes slicing

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
carljm added a commit to carljm/cpython that referenced this issue Jul 5, 2023
* main: (39 commits)
  pythongh-102542 Remove unused bytes object and bytes slicing (python#106433)
  Clarify state of CancelledError in doc (python#106453)
  pythongh-64595: Fix regression in file write logic in Argument Clinic (python#106449)
  pythongh-104683: Rename Lib/test/clinic.test as Lib/test/clinic.test.c (python#106443)
  tp_flags docs: fix indentation (python#106420)
  pythongh-104050: Partially annotate Argument Clinic CLanguage class (python#106437)
  pythongh-106368: Add tests for formatting helpers in Argument Clinic (python#106415)
  pythongh-104050: Annotate Argument Clinic parameter permutation helpers (python#106431)
  pythongh-104050: Annotate toplevel functions in clinic.py (python#106435)
  pythongh-106320: Fix specialize.c compilation by including pycore_pylifecycle.h (python#106434)
  Add some codeowners for `Tools/clinic/` (python#106430)
  pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106423)
  pythongh-61215: Rename `wait_until_any_call` to `wait_until_any_call_with` (python#106414)
  pythongh-106162: array: suppress warning in test_array (python#106404)
  pythongh-106320: Remove _PyInterpreterState_HasFeature() (python#106425)
  pythonGH-106360: Support very basic superblock introspection (python#106422)
  pythongh-106406: Fix _Py_IsInterpreterFinalizing() in _winapi.c (python#106408)
  pythongh-106396: Special-case empty format spec to gen empty JoinedStr node (python#106401)
  pythongh-106368: Add tests for permutation helpers in Argument Clinic (python#106407)
  pythonGH-106008: Fix refleak when peepholing `None` comparisons (python#106367)
  ...
@JosephSBoyle
Copy link
Contributor Author

Closing as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-email type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants