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

Add physicalFilename option into rule tester #14800

Open
JounQin opened this issue Jul 14, 2021 · 9 comments
Open

Add physicalFilename option into rule tester #14800

JounQin opened this issue Jul 14, 2021 · 9 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects

Comments

@JounQin
Copy link
Contributor

JounQin commented Jul 14, 2021

@snitin315 @nzakas @mdjermanovic

I find physicalFilename is not available in ValidTestCase, so it makes testing cases for virtual filename unavailable, should that be supported?

Before context.getPhysicalFilename, I have a custom util getPhysicalFilename and a test case like:

ruleTester.run('remark', remark, {
  valid: [
    {
      code: '<header>Header2</header>',
      parser,
      parserOptions,
      filename: path.resolve(__filename, '0-fake.mdx'), // virtual filename
    },
  ],
})

But when I migrate to context.getPhysicalFilename, it results context.getPhysicalFilename() === filename, it is not correct, so maybe a new physicalFilename option is required for this case:

ruleTester.run('remark', remark, {
  valid: [
    {
      code: '<header>Header2</header>',
      parser,
      parserOptions,
      physicalFilename: __filename,
      filename: path.resolve(__filename, '0-fake.mdx'), // virtual filename
    },
  ],
})

Originally posted by @JounQin in #14616 (comment)

@JounQin JounQin added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jul 14, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jul 14, 2021
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 14, 2021
@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Jul 14, 2021
@nzakas
Copy link
Member

nzakas commented Jul 14, 2021

That seems reasonable to me. @eslint/eslint-tsc thoughts?

@mdjermanovic
Copy link
Member

RuleTester uses Linter API. physicalFilename is not an option on the API. Values returned by context.getFilename() and context.getPhysicalFilename() are calculated from the passed filename and the output of processors applied during the process.

I think it would make more sense to support processors in RuleTester (eslint/rfcs#31), than adding an option that would force Linter to change its internal logic and return some other value from context.getPhysicalFilename().

@nzakas
Copy link
Member

nzakas commented Jul 15, 2021

@mdjermanovic nice catch! That makes sense to me. I completely forgot about the RFC.

@nzakas nzakas added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jul 15, 2021
@nzakas
Copy link
Member

nzakas commented Jul 15, 2021

TSC Summary: This issue requests to add a physicalFilename option to RuleTester to help with testing code blocks. There is already an RFC for enabling processors in RuleTester (eslint/rfcs#31) that may be more appropriate.

** TSC Question:** How do we want to proceed here?

@btmills
Copy link
Member

btmills commented Jul 19, 2021

In the most recent TSC meeting, we agreed to go with the approach described in RFC31. I'll be picking up that RFC to update it as necessary and get it through approval.

@btmills btmills removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jul 19, 2021
@btmills btmills moved this from Feedback Needed to Waiting for RFC in Triage Jul 19, 2021
@github-actions
Copy link

Oops! It looks like we lost track of this issue. @eslint/eslint-tsc what do we want to do here? This issue will auto-close in 7 days without an update.

@nzakas
Copy link
Member

nzakas commented Oct 16, 2021

@btmills where are we on this?

@github-actions github-actions bot removed the Stale label Oct 16, 2021
@btmills
Copy link
Member

btmills commented Oct 20, 2021

It sounded like mysticatea had a couple changes planned for the existing RFC, so I left a comment inquiring what those might be before I make any changes of my own, but I haven't heard back. I'll go ahead and update it for review by the team.

@btmills
Copy link
Member

btmills commented Nov 4, 2021

RFC31 has been updated. Give it a look and please share any feedback!

@btmills btmills moved this from Waiting for RFC to RFC Opened in Triage Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Status: RFC Opened
Triage
RFC Opened
Development

No branches or pull requests

4 participants