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.

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

add build(filename_or_bytes) #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add build(filename_or_bytes) #9

wants to merge 4 commits into from

Conversation

patarapolw
Copy link

@patarapolw patarapolw commented Oct 16, 2018 •
edited
Loading

#1

@wbolster wbolster self-assigned this Oct 29, 2018
@wbolster wbolster requested review from wbolster and a user and removed request for wbolster October 29, 2018 12:42
wbolster
wbolster requested changes Oct 29, 2018
View reviewed changes
Copy link
Collaborator

@wbolster wbolster left a comment •
edited
Loading

Choose a reason for hiding this comment

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

🌳

hey, thanks for your contribution!

a few remarks:

  • i do think the api is confusing, since working of a path object, file pointer, or (byte) string are conceptually different things, yet they're all packed into the same api. this makes it easy to write confusing or broken code.

  • the media type is optional according to rfc2397, yet the proposed implementation requires it

  • tests are missing

  • there are a few unrelated changes that have nothing to do with adding support for building data: uris

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbolster wbolster wbolster requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@wbolster wbolster

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@patarapolw @wbolster