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

Missing mergers: no-caller, no-eval #135

Closed
joeyj-msft opened this issue Aug 5, 2019 · 7 comments Β· Fixed by #227
Closed

Missing mergers: no-caller, no-eval #135

joeyj-msft opened this issue Aug 5, 2019 · 7 comments Β· Fixed by #227
Labels
area: missing merger good first issue Good for newcomers; welcome aboard! status: accepting prs Please, send in a PR to resolve this! ✨

Comments

@joeyj-msft
Copy link

πŸ› Bug Report

  • tslint-to-eslint-config version: dunno. used npx to run
  • ESLint version: 5.16.0
  • Node version: 8.16.0

Actual Behavior

my eslint config is not updated

console output
✨ 101 rules replaced with their ESLint equivalents. ✨
πŸ“’ 4 ESLint rules behave differently from their TSLint counterparts: πŸ“’

  • no-invalid-this:
    • Functions in methods will no longer be ignored.
  • one-var:
    • Variables declared in for loops will no longer be checked.
  • no-plusplus:
    • ++ and -- operators will now only be allowed in for loops.
  • class-methods-use-this:
    • allow-protected methods will no longer be ignored.
      πŸ’€ 2 errors thrown. πŸ’€
      Check ./tslint-to-eslint-config.log for details.
      οΈπŸ‘€ 132 rules do not yet have ESLint equivalents; defaulting to eslint-plugin-tslint. πŸ‘€
      βœ… All is well! βœ…

log file
no-eval threw an error during conversion: Error: No merger for multiple output no-eval rule configurations.
at exports.convertRules (...\npm-cache_npx\14132\node_modules\tslint-to-eslint-config\src\rules\convertRules.js:37:67)
at exports.convertConfig (...\npm-cache_npx\14132\node_modules\tslint-to-eslint-config\src\conversion\convertConfig.js:15:48)
at
at process._tickCallback (internal/process/next_tick.js:189:7)
no-arg threw an error during conversion: Error: No merger for multiple output no-caller rule configurations.
at exports.convertRules (...\npm-cache_npx\14132\node_modules\tslint-to-eslint-config\src\rules\convertRules.js:37:67)
at exports.convertConfig (...\npm-cache_npx\14132\node_modules\tslint-to-eslint-config\src\conversion\convertConfig.js:15:48)
at
at process._tickCallback (internal/process/next_tick.js:189:7)

Expected Behavior

eslint config would be updated

Reproduction

Can you reply to this issue and let me know how much help you need to repro? I'd prefer not to just copy config/details from an internal repo.

@joeyj-msft
Copy link
Author

This was actually changing my eslint config and the errors were non-blocking :)

@JoshuaKGoldberg
Copy link
Member

Oh hi again @joeyj-msft! I'm going to re-open this issue because it's still a bug I'd like to fix. πŸ˜„ thanks for reporting!

Let's assume 0.2.2 as the version.

@JoshuaKGoldberg JoshuaKGoldberg added good first issue Good for newcomers; welcome aboard! status: accepting prs Please, send in a PR to resolve this! ✨ area: missing merger labels Aug 5, 2019
@JoshuaKGoldberg
Copy link
Member

https://github.com/typescript-eslint/tslint-to-eslint-config/blob/master/docs/Architecture.md#rule-mergers describes the root issue here. There are multiple instances of the no-eval and no-callee rules being output, so there needs to be a Merger in code created to handle that.

@joeyj-msft
Copy link
Author

@JoshuaKGoldberg I'm wondering if this could generically handle merging the configs: https://lodash.com/docs/4.17.15#merge

@JoshuaKGoldberg
Copy link
Member

Ah, I'd rather avoid using merge or other forms of generic object merging. There might be rules -either in the core ruleset or some contributed ones- where merging isn't as clean and simple as that. For now, let's be careful and have unfortunately explicit logic per rule.

@joeyj-msft
Copy link
Author

Makes sense. How about an interim approach at outputting the conflicting rules in the log? I'd be happy to take a look at that as the merge will get painful to implement for each case

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Aug 15, 2019

Interesting point. I agree, and that's what it's supposed to be now! πŸ˜† Great observation.

Problem: this is being reported as an error with a full stack. Filed #143 to improve the error UX.

Let's keep this issue, #135, for adding in the missing merger.

@JoshuaKGoldberg JoshuaKGoldberg changed the title rule-name threw an error during conversion: Error: no merger for multiple output rule-name rule configurations. Missing mergers: no-callee, no-eval Oct 7, 2019
@JoshuaKGoldberg JoshuaKGoldberg changed the title Missing mergers: no-callee, no-eval Missing mergers: no-caller, no-eval Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: missing merger good first issue Good for newcomers; welcome aboard! status: accepting prs Please, send in a PR to resolve this! ✨
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants