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 function to apply conversion rules to generated markdown links #55

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

nusbaume
Copy link
Collaborator

@nusbaume nusbaume commented Dec 1, 2023

This PR adds a function to apply header text conversion rules when generating internal markdown document links.

Fixes #54

@nusbaume nusbaume added the bug-fix This PR fixes a bug. label Dec 1, 2023
@nusbaume nusbaume self-assigned this Dec 1, 2023
@nusbaume
Copy link
Collaborator Author

nusbaume commented Dec 1, 2023

Please note that these test failures should be fixed once PR #52 has been accepted and merged (both into main and my branch).

Copy link
Collaborator

@mkavulich mkavulich left a comment

Choose a reason for hiding this comment

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

Looks good! Just had one question.

link_str = text_str.strip()

#Next, make sure all text is lowercase:
link_str = link_str.lower()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check necessary? It appears as if the old links with capital letters still work in the web view, but maybe there's another markdown reader where it doesn't work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good question! After doing some experimentation it looks like at least my web browser (Chrome) is able to use a URL with uppercase text correctly, but I think "under the hood" the actual link is still all lowercase. You can see this if you go to to one of the section heads in the markdown document itself and then click on the the little chain symbol that pops up next to it.

So, I am happy to remove this text conversion if that is preferred by you or others, but I personally think keeping everything lowercase will be the safest option in the long run. Of course please let me know if you disagree!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to leave the check in, I was just curious if it was causing issues.

@mkavulich mkavulich added the needs discussion This PR needs further discussion before being merged label Dec 11, 2023
@nusbaume nusbaume removed the needs discussion This PR needs further discussion before being merged label Dec 15, 2023
@nusbaume
Copy link
Collaborator Author

@mkavulich I removed the "needs discussion" label on this PR because I think you meant to add it to PR #56 instead based off of my memory of Monday's framework meeting discussions. However, if I am wrong here then feel free to add the label back. Thanks!

@mkavulich
Copy link
Collaborator

@nusbaume Yes, you're right I applied the label to the wrong PR. Thanks for fixing that!

@cacraigucar cacraigucar removed their request for review January 3, 2024 20:11
@nusbaume nusbaume merged commit b7524d2 into ESCOMP:main Jan 18, 2024
3 checks passed
@nusbaume nusbaume deleted the fix_section_link_gen branch February 27, 2024 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to apply text-to-link conversion rules in Markdown generation
3 participants