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

Ignore hash (headings) in links to images even in references #76

Closed
wants to merge 2 commits into from

Conversation

kliput
Copy link
Contributor

@kliput kliput commented Aug 7, 2023

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Hi!

The purpose of this feature is to fulfil the following requirement:

When linting image paths in references, hashes should be ignored.

This is follow-up for the PR merged a few years ago: #60

In that PR, I've implemented ignoring hashes in image paths written inline, but now we want to use separate references to images in our project and linter complains about these paths:

obraz

In my PR #60 I've explained why hashes could be ignored in image paths.

I'm open to a discussion and looking forward for a reply :)

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 7, 2023
@kliput kliput marked this pull request as ready for review August 7, 2023 15:04
@wooorm
Copy link
Member

wooorm commented Aug 9, 2023

Hey!

Previously you talked about images.
Now you talk about links.

Why would you have the same use case, but only for links with explicit extensions?
Note that servers are not required to use extensions.
A response can be an image without an extension.
Or a response can be something else, with an “image” extension?

For image nodes, as in your previous PR, I think it makes more sense: folks aren’t putting markdown or HTML files or so in an image src.
But here, we’re talking about href on a, which likely does go to a document, and a deep link when with # at that.

@kliput
Copy link
Contributor Author

kliput commented Aug 9, 2023

Hey,

my use case it to use reference links in images:

![image][image-link]

[image-link]: ./examples/image.jpg#screenshot

This is a use case in the VuePress v1, which is able to set img class basing on the string after the hash sign - here, the generator will add a screenshot class.

Why would you have the same use case, but only for links with explicit extensions?

Because in previous use case, we used inline links in images, and now we need to use links to images stored in references. Unfortunately the link in reference could be used both in images (<img src="...">) or as a link to image (<a href="...">) and I didn't think about it until your message.

Or a response can be something else, with an “image” extension?

Yes, you are right. I've done the implementation for a happy path (a typical usage?), when someone puts an extensions of image at the end, that typically means, that it is an image.

So now, I have doubts if my PR makes sense because of the ambiguity of references - we cannot determine where they are used until we do not check all reference usages. I think, if the reference link is used only as images src, we can ignore everything after # - but this would require a complex implementation.

I'm going to check some alternatives to simply get rid of using # in images links in our project - I will reply about my findings.

@wooorm
Copy link
Member

wooorm commented Aug 9, 2023

We could indeed only apply some behavior to definitions, and then check whether image or link references, reference the definition 🤔

And in the case of both, choose either. Hmmmm

@wooorm
Copy link
Member

wooorm commented Aug 9, 2023

If something is used in an image src, it's very likely an image and not a markdown or html document, whether it's also used as a link too or not. I think I like that fact better than checking extensions

@kliput
Copy link
Contributor Author

kliput commented Aug 9, 2023

I've just discovered, that there is a plugin for Markdown parser that we use, that allows us to apply classes to images without need to use hashes in links. This will be the cleanest solution for us, so I'm going to close this PR, because, as you write - there will be need to implement this in another way, by checking references usage.

Thanks for your involvement!

@kliput kliput closed this Aug 9, 2023
@github-actions

This comment has been minimized.

@kliput kliput reopened this Aug 9, 2023
@kliput
Copy link
Contributor Author

kliput commented Aug 9, 2023

Reopening, in case you have some further comments. You can close it anyway :)

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (1a7f038) 100.00% compared to head (18cce3a) 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #76   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          883       924   +41     
=========================================
+ Hits           883       924   +41     
Files Changed Coverage Δ
lib/find/find-references.js 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wooorm
Copy link
Member

wooorm commented Aug 10, 2023

OK if there’s no clear use case, I’ll close this for now.
If someone finds a concrete use case, we can discuss again!

@wooorm wooorm closed this Aug 10, 2023
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Aug 10, 2023
@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

None yet

3 participants