WomenInSoftwareEngineeringJP / home

Home page for our new organization
https://womeninsoftware.jp/
2 stars 2 forks source link

Code formatting #51

Open sirbully opened 1 week ago

sirbully commented 1 week ago

I notice while working on the coverage issue that we haven't streamlined formatting our code. I'd like to recommend Prettier 🙌 It would also be nice to enforce and automatically handle sort order of our imports.

sirbully commented 1 week ago

@ann-kilzer would like to hear your opinion since both of us will be working on this project a lot so let me know if you have preferences 👀 Also, if it's alright with you, I'd like to pick this up 🤚

ann-kilzer commented 1 week ago

Since eslint has added formatting and stylistic rules, I don't like using prettier. It's another tool to manage and complicates the build steps.

We have some formatting in place. Are there any gaps you notice?

sirbully commented 1 week ago

I format on save which adds semicolons orz I don't think eslint have any power over it (or it doesn't care). While coding, all my single quotes also changed to double quotes and I had to manually change them back to single 🙈 (Nvm, the single quote one is resolved) Not the best dev experience 😭

sirbully commented 1 week ago

Sample of my dev experience, notice it adds semicolons on line 1 and 2

https://github.com/WomenInSoftwareEngineeringJP/home/assets/29673385/64cc83e3-a178-4964-b1fd-bb215c4da8e5

ann-kilzer commented 1 week ago

We may have different people with different developer environments working on the project, and so we have to find a way to align our tooling. I like EditorConfig for a lot of this, and I'll update the eslint rules to have a semi rule. I can look into import order too

The video you share above may be a result of the Prettier plugin. You should disable for this workspace: https://stackoverflow.com/a/75471109/1860768

sirbully commented 6 days ago

The eslint plugins we use at work for import sort order are:

Just sharing for reference! Those two work really well

ann-kilzer commented 5 days ago

@sirbully Thanks for sharing! 🤩

I will have to take more time to look at perfectionist in detail.

simple-import-sort looks interesting and straightforward. If we want something more configurable, I notice https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md . What do you think?

One note with MUI is we should encourage full imports import Component from '@mui/material/Component rather than import { Component } from '@mui/material' (this keeps the bundle smaller) so hopefully whatever import sorting tool aligns with that

sirbully commented 1 day ago

Hey @ann-kilzer, did a little more digging into this.

We moved away from eslint-plugin-import since it didn't support eslint v9 (only up to v8) and typescript-parser v7 (only up to v5). Checking our dependencies, it looks like we use eslint v8 and typescript-parser v7 so eslint-plugin-import will be incompatible with our dev setup.

Upon checking, perfectionist should be good. Apart from satisfying dependency version requirements, it only has minimal dependencies (3) in order for the plugin to work so it won't bloat our project during development.

I also checked simple-import-sort and it only supports typescript-parser v6 so let's not use that 🙇‍♀️

Perfectionist should be super easy to setup! We just need to decide how we want to organize or imports (and exports if you care about that, but from what I noticed we usually export default, so we don't have to setup sorting for exports). No worries about MUI since the importing plugin only checks after the from of the import statement and starts sorting from there. Checking what is imported (full import or otherwise) is out of scope for the sorter.

sirbully commented 1 day ago

Sample setup, docs here: https://eslint-plugin-perfectionist.azat.io/rules/sort-imports#sort-imports

"perfectionist/sort-imports": [
  "error",
  "type": "alphabetical", // optional, default value is alphabetical, can remove this
  "newlines-between": "always", // optional, default value is always, can remove this
  {
    "groups": [
      "react",
      ["builtin", "external"],
      "internal",
      ["parent", "sibling"],
      "style",
    ],
    "custom-groups": {
      "value": {
        "react": ["react", "react-*"]
      }
    }
  }
],
ann-kilzer commented 1 day ago

@sirbully Interesting, thanks for looking into it. It looks like it might be helpful in a larger software project, at the same time, I'd like to keep this codebase lean in terms of features and maintenance. I notice eslint has a built-in import sorting rule: https://eslint.org/docs/latest/rules/sort-imports

Would you think this rule would suffice? Or do you find the grouping to be helpful?

sirbully commented 23 hours ago

@ann-kilzer I personally prefer the grouped sorting as it helps with code readability and I feel that a clear import pattern as early as now contributes to the maintainability of our project in the long term.

We will only need to set this up once so it won't have a lot of overhead and will be as lean as possible 🙇‍♀️

ann-kilzer commented 15 hours ago

Cool, let's give it a try then