Skip to content

Closes #1317: Add filepath to the start event#1874

Open
commodis wants to merge 1 commit into
htmlhint:mainfrom
commodis:1317-file-location
Open

Closes #1317: Add filepath to the start event#1874
commodis wants to merge 1 commit into
htmlhint:mainfrom
commodis:1317-file-location

Conversation

@commodis
Copy link
Copy Markdown

@commodis commodis commented May 15, 2026

I am not well versed in Typescript, but I hope this PR suffices.

Short description of what this resolves

It allows rules to implement a custom filter based on the linted filenames.

Proposed changes

It adds the filepath to the start-event.

@commodis commodis requested a review from coliff as a code owner May 15, 2026 08:54
@coliff coliff requested a review from Copilot May 15, 2026 08:58
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces filepath tracking throughout the HTML hinting process by updating the verify and parse methods and the Block interface. Review feedback highlights that the current modification to the verify method signature is a breaking change for the public API; it is recommended to move filepath to the end of the argument list as an optional parameter to maintain backward compatibility. Further suggestions include making filepath optional in the Block interface to avoid type inconsistencies and providing a default value in the parse method.

Comment thread src/core/core.ts
Comment thread src/cli/htmlhint.ts Outdated
Comment thread src/core/htmlparser.ts Outdated
Comment thread src/core/htmlparser.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #1317 by threading the source filepath through the linting pipeline so that it is exposed on the parser's start event. The CLI's hintFile passes the file path into HTMLHint.verify, which forwards it to HTMLParser.parse, which then includes it on the first start event payload.

Changes:

  • Added a required filepath parameter to HTMLHint.verify, inserted before the optional ruleset argument.
  • Added a required filepath parameter to HTMLParser.parse and a required filepath field on the Block interface; the value is fired with the start event.
  • Updated hintFile in the CLI to forward filepath into HTMLHint.verify.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/core/core.ts Adds filepath parameter to verify and forwards it to parser.parse.
src/core/htmlparser.ts Adds filepath to Block interface and to the start event payload; makes the new parse parameter required.
src/cli/htmlhint.ts Passes the file path from hintFile into HTMLHint.verify.
Comments suppressed due to low confidence (1)

src/core/htmlparser.ts:83

  • No test has been added that asserts the new filepath field is delivered on the start event, even though the rest of test/htmlparser.spec.js extensively covers parser events. Given the linked issue #1317 is specifically about exposing the file path on start, a regression test (e.g. spying on the start event and asserting event.filepath equals the value passed to parse) would help guard against future regressions.
    this.fire('start', {
      pos: 0,
      line: 1,
      col: 1,
      filepath: filepath,
    })

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/core/core.ts Outdated
Comment thread src/cli/htmlhint.ts Outdated
Comment thread src/core/htmlparser.ts Outdated
Comment thread src/core/htmlparser.ts Outdated
@coliff coliff marked this pull request as draft May 15, 2026 09:04
@commodis
Copy link
Copy Markdown
Author

commodis commented May 15, 2026

@coliff which path should I take? I assume a backwards-compatible change is the way to go?


I went the Gemini-Route for now.

@commodis commodis force-pushed the 1317-file-location branch from eb6f4a5 to fce04e8 Compare May 15, 2026 09:11
@commodis commodis marked this pull request as ready for review May 15, 2026 09:18
@commodis commodis force-pushed the 1317-file-location branch from fce04e8 to 8c6504a Compare May 15, 2026 09:19
@commodis
Copy link
Copy Markdown
Author

commodis commented May 15, 2026

@coliff how am I supposed to pass the Ensure no git changes test?

https://github.com/htmlhint/HTMLHint/actions/runs/25910221464/job/76156669308?pr=1874


edit: ah it seems I have to commit the build - I see

@commodis commodis force-pushed the 1317-file-location branch from 8c6504a to 2395343 Compare May 15, 2026 10:03
@commodis
Copy link
Copy Markdown
Author

@coliff I dont think I can pass CodeQL tests? Its reporting code I did not change - as far as I can see.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.

Comment thread src/core/htmlparser.ts
@commodis commodis force-pushed the 1317-file-location branch from 2395343 to a44eddd Compare May 15, 2026 11:36
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