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

Mdlink encoding #340

Merged
merged 3 commits into from
Mar 24, 2022
Merged

Mdlink encoding #340

merged 3 commits into from
Mar 24, 2022

Conversation

YauhenPylAurea
Copy link
Contributor

Continuation of #338

There are 2 issues in the PR that an advise is needed :) Could you please help me with that? Thank you in advance for the help.

  1. Encode func is used from mdurl. It is archived repository without activity. And it seems only encode functionality is required to fix the original issue. So the code was copy-pasted. Would it be better to import the whole mdurl package instead of the selected approach?

  2. Existing test TestOutput has been updated to check the case with a space in a table name. Would it be better to create a completely separate test for such case?

Thank you.

@k1LoW
Copy link
Owner

k1LoW commented Mar 22, 2022

@YauhenPylAurea Thank you!!!!

Would it be better to import the whole mdurl package instead of the selected approach?

Yes, tbls uses gocredits to generate CREDITS. I think it is a good idea to use mdurl as a package to follow that mechanism.
It is also meant to honor that package.

Also, it looks like mdurl is moving to the following repository.

https://gitlab.com/golang-commonmark/mdurl

Would it be better to create a completely separate test for such case?

More test cases are welcome. However, I believe the tests you have modified and added this time are enough.

@YauhenPylAurea
Copy link
Contributor Author

@k1LoW thank you for the review.

Copy-pasted code has been replaced by using mdurl package directly as per your advise. Thanks!

@k1LoW
Copy link
Owner

k1LoW commented Mar 24, 2022

@YauhenPylAurea GREAT!!! Thank you!!!!

@k1LoW
Copy link
Owner

k1LoW commented Mar 24, 2022

Please wait a little longer for the release!

@k1LoW k1LoW merged commit 54ae36a into k1LoW:main Mar 24, 2022
@YauhenPylAurea YauhenPylAurea mentioned this pull request Apr 6, 2022
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.

2 participants