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

Hoist baseline eslint and prettier config for app #53

Closed
Teddarific opened this issue Feb 3, 2021 · 5 comments
Closed

Hoist baseline eslint and prettier config for app #53

Teddarific opened this issue Feb 3, 2021 · 5 comments
Assignees

Comments

@Teddarific
Copy link
Collaborator

We have a baseline eslint and prettier config for the monorepo now.

app will still probably need it's own eslint config to apply specific erb rules it looks like, but we should work towards making the rules consistent with that in server & web. For example, let's work towards turning on no-explicit-any and prefer-default-export etc

Prettier should be more straight forward. There'll be a big change of changing tabWidth from 4 to 2

@alexdanilowicz
Copy link
Collaborator

alexdanilowicz commented Feb 3, 2021

  1. You want to change the electron's app pretitier tabwidth from 4 to 2? Just in the name of consistency to match the server/web repors? The prettier / eslint currently in app is all hijacked from the electron boiler plate repo. We can do this, it will just cause the style github workflow to run every time EDIT: unless we just do a massive prettier write . commit.. seems like overkill IMO?

  2. also — No, I don't want to turn on no-explicit-any because the originally boilerplate code uses any and I'm not sure what type it's suppose to be for a few of them (and clearly the authors didn't either). Same thing for prefer-default-export .... I don't see the value in turning in on now? @Teddarific

@alexdanilowicz
Copy link
Collaborator

alexdanilowicz commented Feb 3, 2021

also, I noticed you've been pushing directly to main. FYI the linting workflow only runs on PR builds https://github.com/Left-on-Read/leftonread/blob/main/.github/workflows/app-format.yml (looks like you haven't done any work on the app so more just FYI). I don't think we should turn it on for main tho cause then we'll get ugly "Github bot" commits on main.

Alternative is we turn that workflow off and use a githook like husky. But I find that people may get impatient and may force skip the pre commit hook if they need to commit quickly.

@Teddarific
Copy link
Collaborator Author

Teddarific commented Feb 4, 2021

@alexdanilowicz

  1. It should be straight forward changing the prettier - I think just deleting the file should do the trick. The big thing will just be a big commit reformatting all the files, but it shouldn't have to be done manually. If we don't already, we should have a command like yarn lint to lint the whole project and --fix any easy things

  2. I mean looking at the use cases of any, there's only one instance where it's the boiler plate, which is inside of assets.d.ts right? That use case is legitimate, and we can just ts-ignore/ eslint-disable-next-line it. I don't feel strongly, but I don't see why not turn these on now if we're going to want to turn these on eventually (which IMO we should). We don't have to fix all the instances now - we can just ts-ignore the current instances if you don't want to fix them now which is totally fine, but it makes it so that if you REALLY need to use any, you have to be very conscious of doing it since you also have to add a ts-ignore

  3. Yeah totally agreed - probably shouldn't turn it on for main. I've been pushing directly to main because I've been linting before I push and once we finish setting up monorepo, i'll start doing PRs but I'm too lazy to right now. This conversation deserves a separate thread, but tldr I changed the default merge to do a rebase, aka no squash. I guess we should do squash huh. I can change it back. But also, I think it would be nice to have a pre-git hook to run the linting locally before pushing, and not depend on a bot to commit for us. Not urgent, but a nice to have for the future so you don't have to pull the bot's changes down.

I'm going to look into hoisting prettier and eslint config for App right now, but I'll leave specific electron eslint configs as is and let you take care of 2)

@alexdanilowicz
Copy link
Collaborator

alexdanilowicz commented Feb 4, 2021

Ok, agreed on all your points. Thanks for typing that out.

re: bot vs git hook. Yeah con of bot is you have to pull down changes, but con of pre-commit hook is you have to wait for it to finish or (if it fails and you're impatient) you run --no-verify. I feel like it's an emacs vs vim thing. We can do whatever you want.

I'll handle # 2, sounds like you are doing # 1.

re: # 3 — squash or bust. (Why do you like rebase? I'm curious? I feel like it clutters commit history.)

@Teddarific
Copy link
Collaborator Author

I'm going to close this issue.

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

No branches or pull requests

2 participants