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

Add a to_markdown method and simplify fix for sections being repeated #108

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jpbalarini
Copy link

@jpbalarini jpbalarini commented Oct 16, 2024

Description

@jpbalarini jpbalarini changed the title Add a to_markdown method and fix repetition of sections Add a to_markdown method and simplify fix for sections being repeated Oct 17, 2024
@jpbalarini
Copy link
Author

@ansukla let me know what do you think

Copy link

@udai-kiran udai-kiran left a comment

Choose a reason for hiding this comment

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

merge

@ansukla
Copy link
Member

ansukla commented Oct 17, 2024

Thank you so much @jpbalarini. This is a great addition to the library.

@jpbalarini
Copy link
Author

jpbalarini commented Oct 23, 2024

@ansukla @udai-kiran is there anything else need from my end to get this PR merged?
Thanks!


# MacOS
*.DS_Store

Choose a reason for hiding this comment

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

I'd argue that this doesn't belong in a project .gitignore, as this is specific to your environment - instead, you should set up a global core.excludesfile. See the relevant GitHub docs.

Copy link
Author

@jpbalarini jpbalarini Nov 16, 2024

Choose a reason for hiding this comment

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

I wouldn't say this is specific to my environment since it's already here in the project and shouldn't be.
*.DS_Store are temporary MacOS files that are created automatically, so anyone contributing to the project could inadvertently add them. So I think it's a good idea to try to avoid that in the future.

@hellerphilipp
Copy link

I like the introduction of this method, but this PR seems to introduce a lot of changes that (a) don't belong here or (b) are a matter of personal style, where you shouldn't overwrite project defaults. An example for (a) is highlighted above (inclusion of .DS_Store in .gitignore), and for (b) I'd call out the replacement of single quotes with double quotes.

I think you have better chances getting the PR merged if you cleanup the PR to forego these personal changes and instead focus on what's essential


html_str = html_str + "</html>"
for child in self.root_node.children:
html_str += child.to_html(include_children=True, recurse=True)

Choose a reason for hiding this comment

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

Thank you for this fix, I was trying to figure out why I was seeing duplicate sections in my HTML.

Copy link
Author

Choose a reason for hiding this comment

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

you're welcome!

@jpbalarini
Copy link
Author

jpbalarini commented Nov 16, 2024

I like the introduction of this method, but this PR seems to introduce a lot of changes that (a) don't belong here or (b) are a matter of personal style, where you shouldn't overwrite project defaults. An example for (a) is highlighted above (inclusion of .DS_Store in .gitignore), and for (b) I'd call out the replacement of single quotes with double quotes.

I think you have better chances getting the PR merged if you cleanup the PR to forego these personal changes and instead focus on what's essential

Thanks for your comments, @hellerphilipp . I replied to the DS_Store file comment above.
Regarding the other comment, the VS Code Black Code plugin automatically changed single quotes for double quotes, taking into account the Python community best practices. If you check any of the files that were changed, they were previously using both single and double quotes in the same file, so I thought it was a good idea to keep those changes and standardize them into one of them.
Happy to get this PR merged so I can make necessary changes if needed.

@hellerphilipp
Copy link

I like the introduction of this method, but this PR seems to introduce a lot of changes that (a) don't belong here or (b) are a matter of personal style, where you shouldn't overwrite project defaults. An example for (a) is highlighted above (inclusion of .DS_Store in .gitignore), and for (b) I'd call out the replacement of single quotes with double quotes.
I think you have better chances getting the PR merged if you cleanup the PR to forego these personal changes and instead focus on what's essential

Thanks for your comments, @hellerphilipp . I replied to the DS_Store file comment above. Regarding the other comment, the VS Code Black Code plugin automatically changed single quotes for double quotes, taking into account the Python community best practices. If you check any of the files that were changed, they were previously using both single and double quotes in the same file, so I thought it was a good idea to keep those changes and standardize them into one of them. Happy to get this PR merged so I can make necessary changes if needed.

Agree that unifying the quotes is probably a good idea. However, it should be its own PR or at least its own commit to separate concerns (and make reviewing less troublesome)

@jpbalarini
Copy link
Author

jpbalarini commented Nov 19, 2024

I like the introduction of this method, but this PR seems to introduce a lot of changes that (a) don't belong here or (b) are a matter of personal style, where you shouldn't overwrite project defaults. An example for (a) is highlighted above (inclusion of .DS_Store in .gitignore), and for (b) I'd call out the replacement of single quotes with double quotes.
I think you have better chances getting the PR merged if you cleanup the PR to forego these personal changes and instead focus on what's essential

Thanks for your comments, @hellerphilipp . I replied to the DS_Store file comment above. Regarding the other comment, the VS Code Black Code plugin automatically changed single quotes for double quotes, taking into account the Python community best practices. If you check any of the files that were changed, they were previously using both single and double quotes in the same file, so I thought it was a good idea to keep those changes and standardize them into one of them. Happy to get this PR merged so I can make necessary changes if needed.

Agree that unifying the quotes is probably a good idea. However, it should be its own PR or at least its own commit to separate concerns (and make reviewing less troublesome)

@hellerphilipp I made the changes you requested and will create a new PR with the style changes after this one gets merged. Thanks!

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

Successfully merging this pull request may close these issues.

5 participants