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

Suggestion data #12606

Closed
mdjermanovic opened this issue Nov 27, 2019 · 4 comments · Fixed by #12635
Closed

Suggestion data #12606

mdjermanovic opened this issue Nov 27, 2019 · 4 comments · Fixed by #12635
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@mdjermanovic
Copy link
Member

The version of ESLint you are using.

6.7.1

The problem you want to solve.

A couple of things I noticed while working on #12591:

  • It isn't documented that a suggestion object should have its own data property for interpolation. This is probably intuitive for most people, but I was still initially expecting the base data object from context.report({ data: {} }) to be used for all messages including suggestions, not just for the main error message.
  • Rule tester doesn't support interpolation for suggestions' messages. You can test either desc (the final text) or just messageId, but not messageId + data. If you pass messageId and data, the tester will check just messageId and ignore data, which is different from how it works with error messages.

Your take on the correct solution to problem.

  • It might be nice to clarify suggestion data in working-with-rules.md.
  • Improve rule tester to use suggestion's data provided in a test in the same way as it works with error's data: interpolate and compare final messages.

Are you willing to submit a pull request to implement this change?

Yes

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon core Relates to ESLint's core APIs and features labels Nov 27, 2019
@mdjermanovic mdjermanovic self-assigned this Nov 27, 2019
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon accepted There is consensus among the team that this change meets the criteria for inclusion labels Nov 27, 2019
@mdjermanovic
Copy link
Member Author

This seems to have enough votes, but since it's a core enhancement I think it needs to be accepted by a TSC member?

@mysticatea
Copy link
Member

To me, this looks like a bug that our tester cannot test suggestions with messageId + data.

@ilyavolodin
Copy link
Member

Agree. I think it's just something that we missed in the original feature PR. Don't think it needs to go through TSC, also it's already up-voted by 3 members of TSC, so I think we should be good.

@mdjermanovic
Copy link
Member Author

mdjermanovic commented Dec 1, 2019

I'm working on the rule tester now. Looks like this issue can be marked as accepted?

There is one problem - the actual version allows desc and messageId in the same test. Even the example has both fields.

This is different from the error tests validation (message and messageId can't be defined on the same error object), but to disallow the same in the suggestion test objects looks like a breaking change now?

desc + messageId anyway doesn't seem to be a confusing combination?

On the other hand, desc + data (+ messageId as it's required for data) is confusing because it defines two description texts in the same test (desc and hydrated).

The solution might be to keep the current behavior (allow desc + messageId and test both if present against the actual result), but disallow desc + data. That shouldn't be a breaking change because data isn't specified in the API and isn't implemented yet.

@ilyavolodin ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 2, 2019
kaicataldo pushed a commit that referenced this issue Mar 18, 2020
…) (#12635)

* Update: Allow testing Suggestions with data in RuleTester (fixes #12606)

* Add data to suggestionObjectParameters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants