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: Code changes & formatting and linting #3552

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Fleny113
Copy link
Contributor

@Fleny113 Fleny113 commented May 6, 2024

I will explain pretty much everything in the PR, but a lot of the changes are the result of running prettier on the entire project and ESLint --fix on most of the files.

The formatting is taking most of the file changes with very minimal ones often but might as well run it one on everything than everytime we need to touch that file (even if by a merge) eslint/prettier edits them and then we got to reverse that change because it's unrelated.

  • .devcontainer/devcontainer.json: Add a yarn install after the dev container is created. Currently if you create a GitHub CodeSpace for example the VSCode task for the build:watch fails as it is missing the node_modules folder.
  • packages/benchmark: This does affect some other files, like .github/workflows/benchmark.yml, the reason this got changed is because while I was testing the benchmarks node (v22 at least) was failing to resolve the benchmark package (the npm package), because it was actually trying to resolve the package itself, so I renamed the package to benchmarks in the package.json, but at that point I have just renamed the folder.
  • .npmignore: On the npm package we have the .tsbuildinfo and we don't need it in a package, so we can just remove it.
  • README.md: Add the bun unit test script, since there was already the deno unit script, might as well add the bun unit script.
  • packages/benchmarks/package.json: The main & exports values were expecting the dist folder to also have the cjs files, but the scripts are to build only with esm, so to make it run i have fixed the values.
  • packages/benchmarks/src/benchmarks/casting.ts: The Camelize 1 event was actually running snakelize on an already snakelized event, so it didn't make any sense.
  • packages/benchmarks/src/benchmarks/memory.ts: The old code was achiving the same thing as snakeToCamelCase, so no reason to use that code instead of the function we already have.
    • also applies to packages/bot/tests/e2e/resetguilds.spec.ts
  • Varius tsconfig.json:
    • tsconfig.test.json: was not getting picked up by typescript (at least in vscode, tested also with a clean codespace) so it got moved to the test directory with the tsconfig.json name and it seems to be getting picked up by typescript.
    • The source tsconfig got edited to explicly state what is the rootDir for the source, use an include pattern instead of an exclude one and set explicitly what is the path for the .tsbuildinfo file.
    • packages/tsconfig/base.json: The exclude pattern is actually relative to where the base.json is, so it wasn't doing anything. It will be the case untile TS 5.5 where we will be able to use ${configDir} to set things relative to the destination tsconfig.json, so most tsconfig.json will provably become just a json with an export like the tests ones. TS 5.5 Beta, ${configDir}. The skipDefaultLib is just skipping the check for typescript .d.ts files.
  • packages/gateway/tests/integration/connection.spec.ts: ESLint warnings/errors mainly
  • scripts/checkCpuModel.js: ESLint error
  • packages/rest/tests/e2e/utils.ts: This affected most if not all e2e files, just a rename to use camelCase instead of all lowercase.
  • scripts/finalizeTypedocs.js: Node 22 compatibility, node 22 removed support for assert in imports statements and the import function, so it broke when run with node 22. This does not affect node 18+ as it seems to be working under a built of the latest LTS of node 18.
  • Varius package.json:
    • packages/benchmarks/package.json: microtime is not used, node-fetch is not necessary considering we support only node 18+ where the built-in fetch got stable.
    • packages/rest/package.json: We do use dotenv for our tests (to use the .env file in the root of the directory in e2e tests) but we don't need to specifiy it in the "prod" dependencies. why-is-node-running is not used, and doesn't seem to have a reason to be there either
    • packages/types/package.json: In the types package we just have an "import correctly" test, so chai is not necessary, also @types/simon without simon installed is not doing anything, just creating clutter in the package.json.
    • packages/utils/package.json: While we might want to readd the public key validation for HTTP interactions (from what I understand, at some point we had the code to do support this), currently we don't need tweetnacl to be installed.

If there is something that isn't clear why it got edited, i will do my best to respond with a reason, reason that might just prettier/eslint that edited or why i did edit it.

@Fleny113 Fleny113 requested review from H01001000 and a team as code owners May 6, 2024 20:49
@github-actions github-actions bot added w-unverified This has not been verified pkg-all Affects every package pkg-bot Affects the bot package pkg-gateway Affects the gateway package pkg-rest Affects the rest package pkg-types Affects the types package pkg-utils Affects the utils package t-ci Changes to our CI configuration files and scripts website Affects the website labels May 6, 2024
@Fleny113 Fleny113 self-assigned this May 6, 2024
@Fleny113 Fleny113 added t-refactor A code change that neither fixes a bug nor adds a feature t-style Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, et t-build Changes that affect the build system or external dependencies github_actions Pull requests that update GitHub Actions code labels May 6, 2024
@Fleny113 Fleny113 enabled auto-merge May 7, 2024 13:12
Copy link
Member

@lts20050703 lts20050703 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code pkg-all Affects every package pkg-bot Affects the bot package pkg-gateway Affects the gateway package pkg-rest Affects the rest package pkg-types Affects the types package pkg-utils Affects the utils package t-build Changes that affect the build system or external dependencies t-ci Changes to our CI configuration files and scripts t-refactor A code change that neither fixes a bug nor adds a feature t-style Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, et w-unverified This has not been verified website Affects the website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants