search

FelixSchwarz / mjml-python

Python implementation for MJML - a framework that makes responsive-email easy
MIT License
73 stars 16 forks source link

mjml_to_html(string) #15

Closed buttle closed 3 years ago

buttle commented 3 years ago

Hi,

Relevant to issue #4 here is a proposal.

Please let me know what you think.

Thanks.

FelixSchwarz commented 3 years ago

Thank you very much for your contribution.

I have a few comments:

buttle commented 3 years ago

Why not use elif isinstance(xml_fp_or_json, str) and then xml_fp = StringIO(xml_fp_or_json)? That way we would normalize everything to file-like instances without an extra condition.

It's an ugly hack. Sorry 'bout that. :( A bit like the other one I did some months back.

It would be nice to have at least one test. You can check 5c257c9 for an example. (If you don't feel comfortable writing the test you can leave that to me.)

I don't have a lot of experience writing tests, yet.

The commit message needs to explain what you are doing and why. For example: mjml_to_html(): also accept plain strings as input

In another project I'm involved with a CONTIBUTING file is under construction.

Thanks again for this library :)

FelixSchwarz commented 3 years ago

Why not use elif isinstance(xml_fp_or_json, str) and then xml_fp = StringIO(xml_fp_or_json)? That way we would normalize everything to file-like instances without an extra condition.

It's an ugly hack. Sorry 'bout that. :( A bit like the other one I did some months back.

No problem. Nobody starts out writing perfect code. Would you mind reworking your pull request along my suggestion above?

It would be nice to have at least one test. You can check 5c257c9 for an example. (If you don't feel comfortable writing the test you can leave that to me.)

I don't have a lot of experience writing tests, yet.

Ok, I'll add a test when you reworked the if/elif/else condition as mentioned above. I can only recommend learning about automated tests though. This is a really important technique to keep bigger projects (or a lot of small ones) maintainable.

The commit message needs to explain what you are doing and why. For example: mjml_to_html(): also accept plain strings as input

In another project I'm involved with a CONTIBUTING file is under construction.

Not sure what you meant by that. Do you mean this project should have a contribution guidelines? If so please propose what I should write there.

buttle commented 3 years ago

Would you mind reworking your pull request along my suggestion above?

Ok. Done!

This is a really important technique to keep bigger projects (or a lot of small ones) maintainable.

Yep. It's high on my list. :)

Do you mean this project should have a contribution guidelines? If so please propose what I should write there.

If you prefer commit messages to have some format, you could mention that in the CONTRIBUTING.md file. There's lots of examples to get ideas from. It's pretty much up to you. This one is extensive https://github.com/angular/angular/blob/master/CONTRIBUTING.md

FelixSchwarz commented 3 years ago

cherry-picked (just omitted the whitespace changes) in e7798368621864c00be1b51c9e7d68d1b98354fc

Thank you very much for your contribution.

If you prefer commit messages to have some format, you could mention that in the CONTRIBUTING.md file.

This is not so much about a specific format. The main idea is that the commit message must contain why a certain change was made. I mean everyone can see what was changed when checking the diff. If the what is already quite long, you should probably create multiple commits. However the really important part is the rationale for the change:

I think this is something which the Linux kernel community (mostly) got right a long time ago. I recommend reading the Linux guidelines about commit messages to every developer.