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

Refactor/simplify internal link check resolution #795

Conversation

riccardoporreca
Copy link
Collaborator

Following some deep dive into the internal link check resolution from #792 (comment), I am proposing here a improved and simplified approach that also removes some legacy historical logic.

This also includes:

  • An initial refactoring of the source and filename information as URL attributes, which is more natural given the role they play in checking internal links, compared to Runner attributes that need to be consistently set and complicate code understanding/maintenance.
  • Linting the code in the README for consistent and up-to-date standards.

More details can be found in individual commit for better assessment of individual changes, with commit messages providing more information.

* They are needed in `URL` methods to check internal links, and this way we avoid having to set them as `Runner` attributes while looping over collected links.
* This also allows a more consistent definition and usage of internal links metadata.
* So we can extract `line` and `content` from there instead of explicitly passing them every time.
* Most of the logic is legacy from the very early HTMLProofer, and is irrelevant not since `@filename` can only be the file where the link is defined, with `File.join` properly handling both same-directory, nested, and parent links, together with the ultimate `File.expand_path` in `absolute_path.
* The legacy logic comes pretty much from gjtorikian#6 and gjtorikian#23.
* This way we can also avoid `File.exist`, which can ultimately be delegated to checking the existence, not constructing the path.
* We now construct and maintain a hash of resolved paths, as a way to have a single instance of going through OS for checking the existence of alternative resolved paths, including assumed extension and directory index file.
@riccardoporreca
Copy link
Collaborator Author

The additional commit d52d828 above provides a larger refactoring aiming at a single-point interaction with the OS for resolving and checking existence of the same absolute_path.

@gjtorikian
Copy link
Owner

@riccardoporreca Would you like to have push rights to this repository? I fear I can no longer devote time to it and your work has been exemplery. I glanced through this PR and the only thing that matters to me is that the tests pass. As it does that, this looks great.

@riccardoporreca
Copy link
Collaborator Author

@gjtorikian

Would you like to have push rights to this repository? I fear I can no longer devote time to it and your work has been exemplery.

I don't mind to keep going via PRs from my fork and would in any case go through the PR review process. However I understand your limited availability for this project might benefit from having a second, more active person with push rights and would not mind that. Up to you!

@gjtorikian gjtorikian merged commit 8fd0ddc into gjtorikian:main Mar 15, 2023
3 checks passed
@gjtorikian
Copy link
Owner

I sent you a collab invitation. Thank you again very much for this PR.

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