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

fix: resolve CI failures #30

Closed
wants to merge 5 commits into from
Closed

fix: resolve CI failures #30

wants to merge 5 commits into from

Conversation

KyleKing
Copy link
Collaborator

When the URL scheme is present, the URL is recognized and parsed, which may have once worked on older versions of linkify, but is not working today. There aren't logs to confirm, but the historic CI failures go back to 2021, but the logs have expired: https://github.com/hukkin/mdformat-gfm/actions/runs/3567981113?pr=20

I think it's reasonable to make this change because the test is verifying that the autolink extension is run and not how the extension is implemented

@KyleKing KyleKing mentioned this pull request Jan 10, 2024
@KyleKing
Copy link
Collaborator Author

Update: so the problem seems to be that with the latest versions of mdformat and/or linkify, the backslash is splitting the URL into two. Using a different character, such as % is encoded, but that breaks the CLI test because it produces different HTML.

https://github.com/KyleKing/mdformat-gfm/blame/735781e3fcf95d92cd4537a9712893d00415cd63/tests/data/default_style.md#L65

My guess is that the backslash was a nice hack at the time to verify that the content was being formatted, but now that there have been upstream changes, that hack no longer works. Walking through the relevant logic, the href only includes the first half of the URL:

# Input: `www.python.org/autolink\extension`
(Pdb) autolink_url = node.attrs["href"]
(Pdb) autolink_url
'http://www.python.org/autolink'
(Pdb) node.children[0].content
'www.python.org/autolink'
(Pdb) autolink_url.split(":", maxsplit=1)[1]
'//www.python.org/autolink'
# which all works expected other than missing the backslash

I'm not sure if there is a use-case where a user shouldn't manually fix the URL containing unencoded characters and there haven't been any bugs reports, so I think this is safe to merge

@KyleKing
Copy link
Collaborator Author

KyleKing commented Jan 11, 2024

CI failed because of a time out for one of the combinations, but there was no clear issue (the steps are all about 1 minute). Could be a bug related to how awaiting approval works?

build (3.11, macos-latest)
The job running on runner GitHub Actions 6 has exceeded the maximum execution time of 360 minutes.

Testing intermediary Python versions shouldn't be necessary, so I slimmed down the matrix to just the min and max supported Python version, which is currently passing: https://github.com/KyleKing/mdformat-gfm/actions/runs/7495323106 (all: https://github.com/KyleKing/mdformat-gfm/actions)

On a side-note, I tried bumping the upper Python version to 3.12, but ran into issues installing pre-commit (in particular the pyyaml dependency) on Windows with Python 3.12 (one alternative is to change how pre-commit is installed, e.g. just when used, but that's out of scope)

@hukkin
Copy link
Owner

hukkin commented Jan 30, 2024

Sorry for being unresponsive.

Thanks! The effort is much appreciated. I wanted to make sure that the upstream changes do not change Markdown AST. It seems that isn't the case.

Previously www.python.org/autolink\extension was interpreted as an autolink (by linkify-it), but now it isn't. My interpretation of the GFM spec is that it shouldn't be (the backslash and whatever follows, that is). If prefixed by http:// it should be a valid link though, I've updated the test accordingly and named it better.

CI seems to work fine for me so I kept it as is.

@hukkin hukkin closed this Jan 30, 2024
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.

None yet

2 participants