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

Support Links with parenthesis #73

Merged
merged 2 commits into from
Apr 7, 2023

Conversation

Richard-Gist
Copy link
Contributor

PROBLEM

Ink does not currently parse URLs with parenthesis () correctly, because it reads until the first closing parenthesis ).

Example

Wikipedia has numerous links like this.
Github parsing: markdown example
Raw markdown: [markdown example](https://en.wikipedia.org/wiki/Service_(systems_architecture))

SOLUTION

Use a balancing approach and count how many new opening parenthesis ( appear to ensure that reading continues until the final closing parenthesis is encountered.

Other solutions

I saw that there was already a PR that was a couple years old by nickd that hadn't been merged. It took the approach of counting opening parenthesis after reading ended, then continue reading until the final parenthesis is closed (accounting for new opening parenthesis).

I tried my tests on both solutions, and they both pass. I also ran some performance tests and they are essentially equal (0.0000X difference) in that regard.

If that solution is merged instead, I'm happy to close this PR.

Feel free to make any changes you want to the PR.

@Richard-Gist
Copy link
Contributor Author

I believe the failure in Ink-Linux is unrelated to my change.

I'm not sure how to update bitrise to fix the issue in Ink-Linux, but it appears to need a newer version libcurl.

+------------------------------------------------------------------------------+
| (2) Do anything with Script step                                             |
+------------------------------------------------------------------------------+
| id: script                                                                   |
| version: 1.1.5                                                               |
| collection: https://github.com/bitrise-io/bitrise-steplib.git                |
| toolkit: bash                                                                |
| time: 2022-11-08T22:20:15Z                                                   |
+------------------------------------------------------------------------------+
|                                                                              |
+ apt-get install libcurl3
Reading package lists...
Building dependency tree...
Reading state information...
Package libcurl3 is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source
However the following packages replace it:
  libcurl4
E: Package 'libcurl3' has no installation candidate
|                                                                              |
+---+---------------------------------------------------------------+----------+
| x | Do anything with Script step (exit code: 100)                 | 0.81 sec |
+---+---------------------------------------------------------------+----------+
| Update available: 1.1.5 -> 1.2.0                                             |
|                                                                              |
| Release notes are available below                                            |
| https://github.com/bitrise-io/steps-script/releases                          |
| Issue tracker: https://github.com/bitrise-io/steps-script/issues             |
| Source: https://github.com/bitrise-io/steps-script                           |
+---+---------------------------------------------------------------+----------+

@mflint
Copy link

mflint commented Mar 27, 2023

This looks like a useful contribution from @Richard-Gist.

@JohnSundell Any chance of these open PRs getting a little love, please?

Copy link
Owner

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

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

@Richard-Gist I'm so sorry for taking so ridiculously long to review this PR. It's an excellent addition to the library, and I think you solved the problem in a very elegant way. I also appreciate how you included context, such as performance testing (which is super important for Ink), in your PR description. I'll sort out the CI issues, and will get this merged once that's done. Thanks a lot for contributing!

@JohnSundell JohnSundell merged commit 70b4f8b into JohnSundell:master Apr 7, 2023
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.

3 participants