Add ESLint and Prettier to py-slang by AaravMalani · Pull Request #88 · source-academy/py-slang

AaravMalani · 2026-03-06T17:09:11Z

Solves issue #84
There are some manual type changes ~~and TODOs left for types who I've not yet moved from any~~, such as moving Value back to a union of NumberValue, StringValue, etc.

.prettierrc

…ove tests from ESLint

AaravMalani · 2026-03-07T12:56:29Z

Everything's set for review, but should we include src/tests in ESLint?
ESLint doesn't accept it since it's excluded in the tsconfig.json, and Rollup fails if it builds the files in src/tests. If I could disable src/tests in Rollup, I'd be able to reinclude src/tests in tsconfig.json so I could test with ESLint, but I can't figure out how to disable it

...
typescript({ exclude: ["src/tests/**/*.ts"] })
// and
typescript({ exclude: ["src/tests"] })
// and
typescript({ exclude: ["src/tests/*.ts"] })
...

None of the above options worked for me, please let me know if there's a way to do it I'm not thinking of

RichDom2185 · 2026-03-07T19:19:17Z

package.json

+    "format": "prettier --write \"src/**/*.{ts,tsx}\"",
+    "format:ci": "prettier --list-different \"src/**/*.{ts,tsx}\"",


Let's format . instead of src to include package.json, eslint config etc. (which is currently improperly formatted)

Add a .prettierignore file so it excludes the docs folder.

RichDom2185 · 2026-03-07T19:20:19Z

eslint.config.mjs

+  tseslint.configs.recommended,
+  eslintConfigPrettierFlat,
+  {
+    files: ['**/*.ts*'],


scope to src/**/*.{ts,tsx}, this scans way too many files

I disagree, since it runs on src in the command anyways, but I agree if anyone runs it without npm run lint, it would be painful

RichDom2185 · 2026-03-07T19:20:41Z

tsconfig.json

+    "rollup.config.js",
    "src/tests",
-    "rollup.config.js"
+    ".prettierrc",


Shouldn't need this, it's not a JS/TS file

Ah, sorry about that
I forgot to remove them when I was done testing for the src/tests bug I'm encountering

RichDom2185 · 2026-03-07T19:20:47Z

tsconfig.json

    "src/tests",
-    "rollup.config.js"
+    ".prettierrc",
+    "eslint.config.mjs"


Why are we excluding it?

Oh I remember why, it's because Typescript threatened to overwrite them for some reason
I think it makes sense to just restrict Typescript to src, since no other Typescript files are present anywhere else in py-slang

RichDom2185 · 2026-03-07T19:20:56Z

tsconfig.json

    "jest.config.js",
    "node_modules",
    "dist",
+    "rollup.config.js",


Ditto, why are we excluding it?

RichDom2185 · 2026-03-07T19:22:46Z

eslint.config.mjs

+    }
+  },
+  {
+    files: ['**/*.js'],


Again, this is too huge. This also gives a subtle problem with code fragility because you are running tseslint on files like ESLint and Rollup config, but without the project service, so not all rules may be applicable (aka it just "happens to" work for this case, not "guaranteed" to work for all cases).

I've restricted it to src/**/*.js (although for now, that's not gonna be used for a while) for now
Isn't the project service a typescript-eslint feature IIRC?
Hope this solves the issue, though!

RichDom2185 · 2026-03-07T19:23:09Z

package.json

+    "eslint": "^9.39.3",
+    "eslint-config-prettier": "^10.1.8",
+    "eslint-plugin-import": "^2.32.0",


Lint and format, etc, should all be devDependencies, not dependencies.

Huh, I thought I used -D
Thanks for pointing it out!

RichDom2185 · 2026-03-07T19:30:33Z

Everything's set for review, but should we include src/tests in ESLint? ESLint doesn't accept it since it's excluded in the tsconfig.json, and Rollup fails if it builds the files in src/tests. If I could disable src/tests in Rollup, I'd be able to reinclude src/tests in tsconfig.json so I could test with ESLint, but I can't figure out how to disable it
...
typescript({ exclude: ["src/tests/**/*.ts"] })
// and
typescript({ exclude: ["src/tests"] })
// and
typescript({ exclude: ["src/tests/*.ts"] })
...
None of the above options worked for me, please let me know if there's a way to do it I'm not thinking of

Sorry, what do you mean?

ESLint doesn't accept it since it's excluded in the tsconfig.json

ESLint should ideally lint test files too (may or may not use a different ruleset compared to source files); why would it not accept it just because it's excluded in tsconfig.json? ESLint and tsconfig.json are two separate things
Rollup fails if it builds the files in src/tests

Yeah makes sense, Rollup shouldn't build tests in the first place.
I'd be able to reinclude src/tests in tsconfig.json so I could test with ESLint

You don't have to include src/tests in tsconfig.json to lint it with ESLint. They are two completely separate things
but I can't figure out how to disable it

Disable what?

AaravMalani · 2026-03-08T02:26:39Z

ESLint should ideally lint test files too (may or may not use a different ruleset compared to source files); why would it not accept it just because it's excluded in tsconfig.json? ESLint and tsconfig.json are two separate things

No, they aren't because typescript-eslint accepts tsconfig.json to know the project in parserOptions, so it also accepts the exclusion list

...
    languageOptions: {
      parser: tseslint.parser,
      parserOptions: {
        project: './tsconfig.json',
      },
    },
...

If I include the tests, it errors out with

/root/source-academy/py-slang/src/tests/wasm-compiler.spec.ts
  0:0  error  Parsing error: "parserOptions.project" has been provided for @typescript-eslint/parser.
The file was not found in any of the provided project(s): src/tests/wasm-compiler.spec.ts

Disable what?

The inclusion of src/tests in the Rollup build, sorry I wasn't clear
Although now that I think about it, maybe creating a separate tsconfig.eslint.json which extends from tsconfig.json but includes the tests could work, I'll have to test that

…t not working on tests + Move linter + formatters to `devDependencies`

AaravMalani · 2026-03-08T02:51:41Z

ESLint should ideally lint test files too (may or may not use a different ruleset compared to source files); why would it not accept it just because it's excluded in tsconfig.json? ESLint and tsconfig.json are two separate things

No, they aren't because typescript-eslint accepts tsconfig.json to know the project in parserOptions, so it also accepts the exclusion list
...
    languageOptions: {
      parser: tseslint.parser,
      parserOptions: {
        project: './tsconfig.json',
      },
    },
...
If I include the tests, it errors out with
/root/source-academy/py-slang/src/tests/wasm-compiler.spec.ts
  0:0  error  Parsing error: "parserOptions.project" has been provided for @typescript-eslint/parser.
The file was not found in any of the provided project(s): src/tests/wasm-compiler.spec.ts
Disable what?

The inclusion of src/tests in the Rollup build, sorry I wasn't clear Although now that I think about it, maybe creating a separate tsconfig.eslint.json which extends from tsconfig.json but includes the tests could work, I'll have to test that

Nevermind, I think projectService.allowDefaultProject was what I was looking for
Thanks for your help, though!

AaravMalani added 2 commits March 7, 2026 01:06

Add ESLint and Prettier + make formatting changes

c50331e

Add linting to the workflow

56758fa

RichDom2185 requested changes Mar 6, 2026

View reviewed changes

.prettierrc Show resolved Hide resolved

AaravMalani added 2 commits March 7, 2026 18:19

Add format to CI

2e64572

Fix the remaining types + make Value types stricter + temporarily rem…

59d96d6

…ove tests from ESLint

AaravMalani marked this pull request as ready for review March 7, 2026 12:42

RichDom2185 requested changes Mar 7, 2026

View reviewed changes

Add prettierignore + Extend prettier to entire repository + fix ESLin…

2d993b9

…t not working on tests + Move linter + formatters to `devDependencies`

Fix formatting of eslint.config.mjs

169480d

martin-henz requested a review from mug1wara26 March 9, 2026 01:00

		"format": "prettier --write \"src/*/.{ts,tsx}\"",
		"format:ci": "prettier --list-different \"src/*/.{ts,tsx}\"",

Conversation

AaravMalani commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AaravMalani commented Mar 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichDom2185 commented Mar 7, 2026

Uh oh!

AaravMalani commented Mar 8, 2026

Uh oh!

AaravMalani commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AaravMalani commented Mar 6, 2026 •

edited

Loading

AaravMalani commented Mar 8, 2026 •

edited

Loading