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

Fix parsing errors #1

Merged
merged 14 commits into from
Sep 20, 2023
Merged

Fix parsing errors #1

merged 14 commits into from
Sep 20, 2023

Conversation

bryan-hoang
Copy link
Member

@bryan-hoang bryan-hoang commented Aug 27, 2023

I didn't thoroughly test the plugin on a large code base, which led to spurious errors from the plugin failing to parse edge cases (e.g., failing to parse const path = require('node:path');). To address the issues, I decided to improve the testing DX by switching from mocha to vitest before moving on to tackling the issues.

In addition to adding multiple test cases and CI checks, I opted to use https://github.com/eslint/eslint/blob/main/lib/rules/brace-style.js as a reference to modify it to fit the brace style of the company's style guide.

@bryan-hoang bryan-hoang force-pushed the bugfix/loc-is-undefined branch 9 times, most recently from 5eb48db to ae83e2b Compare August 28, 2023 00:35
@bryan-hoang bryan-hoang self-assigned this Aug 28, 2023
@bryan-hoang bryan-hoang added the bug Something isn't working label Aug 28, 2023
@bryan-hoang
Copy link
Member Author

@j0ashm Feel free to skim it to see the changes if ya have the time. I should've done a better job testing the plugin on our monorepo 😅.

@bryan-hoang bryan-hoang marked this pull request as ready for review August 28, 2023 00:46
@bryan-hoang bryan-hoang changed the title Fix plugin errors Fix parsing errors Aug 28, 2023
@j0ashm
Copy link
Contributor

j0ashm commented Aug 28, 2023

@bryan-hoang Out of curiosity, what kind of edge cases were missed? Reading the issue, I'm not quite sure where/why node:path comes into play as a parsing error?

@j0ashm
Copy link
Contributor

j0ashm commented Aug 28, 2023

I do remember running the plug-in on the DCP monorepo and it didn't throw up any errors... maybe I had it misconfigured?

@bryan-hoang
Copy link
Member Author

bryan-hoang commented Aug 28, 2023

@bryan-hoang Out of curiosity, what kind of edge cases were missed? Reading the issue, I'm not quite sure where/why node:path comes into play as a parsing error?

@j0ashm If you're on the main branch and add const path = require('node:path'); as a test cast, the test will fail. e.g., apply test-case.patch and run the tests on main.

I do remember running the plug-in on the DCP monorepo and it didn't throw up any errors... maybe I had it misconfigured?

After linking the plugin & config in, I essentially ran npx -- eslint --ignore-path=.gitignore --ext=js,simple,tap,simple.failing,tap.failing ., and ran into the error on attic/bank/bin/bank.js:3, which is the failing test case.

Copy link

@edeluzio edeluzio left a comment

Choose a reason for hiding this comment

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

A couple nitpicks, but overall LGTM. Running without problems locally

.eslintrc.js Show resolved Hide resolved
docs/rules/brace-style.md Outdated Show resolved Hide resolved
Fixes lint issues & adds relevant links/examples.
In additional to switching to `messageId`, additional lint rules are
also added and followed.
Encountered when trying to use the plugin on a large code base.
Updates the minimum node version to 18 to address the EOL for security
updates of Node 16 on 2023-09-11.

Commands ran to update the `package*.json` files:

```console
npm pkg set engines.node='>=18'
npm install --package-lock-only
```

Also fixed audit issues with `npm audit fix`.

Refs: https://nodejs.dev/en/about/releases/
The forked version is maintained.
Mainly for a better DX.
Added a lot more test cases and refactored the custom rule to be based
on the core `brace-style` rule from ESLint.
Makes it easier to configure the plugin.
In addition to adding the config, lint issues were addressed.
Make sure `package.json` is used as the single source of truth for the
package's name & version.
The plugin uses `context.souceCode` which was only recently added in May
this year.

If the version of the peer dependency isn't satisfied, the plugin breaks
when the property is undefined.

Refs: https://eslint.org/blog/2023/05/eslint-v8.40.0-released/#highlights
@bryan-hoang bryan-hoang requested review from edeluzio and removed request for wiwichips September 19, 2023 13:20
@bryan-hoang bryan-hoang force-pushed the bugfix/loc-is-undefined branch 2 times, most recently from 13221ea to 0a75b56 Compare September 19, 2023 13:28
@edeluzio
Copy link

Good change with the link, it's minor but better IMO.
LGTM!

Copy link

@edeluzio edeluzio left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@eroosenmaallen eroosenmaallen left a comment

Choose a reason for hiding this comment

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

Looks right

@bryan-hoang bryan-hoang merged commit 1cc3826 into main Sep 20, 2023
2 checks passed
@bryan-hoang bryan-hoang deleted the bugfix/loc-is-undefined branch October 13, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants