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

chore: Add ESLint-plugin-ejs-js #2492

Closed
wants to merge 1 commit into from
Closed

Conversation

nschonni
Copy link
Collaborator

@nschonni nschonni commented Jan 13, 2021

Allow ESLinting of EJS files

@nschonni
Copy link
Collaborator Author

@peterbe think I'm running into a similar oddity that I hit with the flattening of the shell scripts. I can also just run the cleanups and submit them as a first step

@peterbe
Copy link
Contributor

peterbe commented Jan 13, 2021

I have an love-hate relationship with this PR. There appears to be a bunch of useful fixes eslint uncovered. But I definitely don't like the added complexity of having to rely on patches and postinstall things. Just because it's possible, doesn't mean we should do it.
If anything, keep the edits that eslint proposes but run that separately. When the 3rd-party tools get fixes, we can put the tools in. Or, can you make the necessary edits so the patches aren't even needed?

@nschonni
Copy link
Collaborator Author

@peterbe I'm going to submit the fixes in separate PRs since it seems like they are OK.

I did submit the hack upstream, but it seems like it's a plugin the author never ended up using. I'll also take a look if the whole parser can be vendored into this project and locally loaded by ESLint

@peterbe
Copy link
Contributor

peterbe commented Jan 13, 2021

For the record, once a long time ago, before we could decide between ESLint or TypeScript for all the Node code, I didn't want to wait so I installed eslint locally (one-off) and applied a bunch of fixes. It got use 90% more "correct" but obviously not sustainable.
When I felt the TypeScript momentum started losing steam, that's when I put in a really basic eslint config just to get have something a little big greater than 0.
I still think TypeScript might be best, but it's hard to justify the time-spend. Especially when Yari was so young where you probably want to change directions faster rather than getting the reliability and inertia that TypeScript offers.

@nschonni
Copy link
Collaborator Author

Even for Typescript, the TSLint library ended up porting to a ESLint plugin+parser 😄

@nschonni
Copy link
Collaborator Author

I split out some of the cleanups to separate PRs. After those land, I'll rebase this to do a bigger sweap of the unused variables and redefined variables as new PRs.

@nschonni
Copy link
Collaborator Author

@peterbe so apparently vendoring the plugin into the repo works 😆

@nschonni nschonni force-pushed the ejs-lint branch 3 times, most recently from 40253a7 to bd5e4bd Compare January 15, 2021 18:29
@nschonni
Copy link
Collaborator Author

@peterbe I rebased out the template changes since they mostly got spun out to separate PRs

@nschonni nschonni force-pushed the ejs-lint branch 6 times, most recently from 25f4891 to 11d942a Compare January 20, 2021 22:01
@nschonni
Copy link
Collaborator Author

@escattone fyi, this is the base that I've been using to find the EJS stuff

@nschonni
Copy link
Collaborator Author

Rebased and added in the one-var ignore for now, with a fix for that in #2729

@peterbe
Copy link
Contributor

peterbe commented Jan 29, 2021

Drive-by-comment; Is this helping? A lot of the code in the .ejs files is horrible. And we're aware of that. It's old and not very well maintained and honestly, we're hoping it can just last a little bit longer until we get rid of it.

Also, vendoring in code is scary to me. Now it shows up when you search the code and I don't even know what it does to our licensing language when we have to mention that portions are different.

@escattone
Copy link
Contributor

@nschonni I'm sorry for the delay in getting to this. I'm sorry to say that I don't think this is worth adding. I'm sorry because you've done a lot of good work, and I have nothing but respect and admiration for that and your generousity! However, our goal this year is to remove, render-out, or replace all or nearly all of the macros, so I'd rather not invest any more effort and/or code down this path. With that said though, I don't mind that you're using this to create PR's for cleaning-up the current state of our macros (although those PR's will be considered low priority given everything else we have to do). I'm going to close this, but not without admiration for your work!

@escattone escattone closed this Feb 5, 2021
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.

None yet

3 participants