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

I can't get linting to work #370

Open
zepumph opened this issue Feb 21, 2023 · 8 comments
Open

I can't get linting to work #370

zepumph opened this issue Feb 21, 2023 · 8 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Feb 21, 2023

While working on #366 (comment), I found that I could not get any lint errors to show up from the rosetta repo.

In find-bad-pattern-translations, I added a couple problems and could not get them to show up. @liammulh can you help me please?

@zepumph
Copy link
Member Author

zepumph commented Feb 21, 2023

In addition, we have another problem where some other extensions like mjs and cjs aren't supported by our linter. I'm happy to add ".mjs" and ".cjs" file extensions to our config in lint.js, but when I do, there are 50 other lint errors in the Gruntfile and eslintrc in rosetta, so I can't commit that at this time. Just know those files aren't being linted right now.

zepumph added a commit to phetsims/perennial that referenced this issue Feb 21, 2023
zepumph added a commit to phetsims/chipper that referenced this issue Feb 21, 2023
@zepumph
Copy link
Member Author

zepumph commented Feb 21, 2023

While poking around I found that some linting refactoring I did last year broke grunt lint in perennial, fixed above.

@liammulh
Copy link
Member

I found that I could not get any lint errors to show up from the rosetta repo

@samreid and I tested this when we worked on 30967d1, and when we created a lint error in Rosetta and then ran the lint-everything task in Perennial, we saw an error.

@zepumph, are you saying that when you run Perennial's lint-everything task, you can't see lint errors in Rosetta?

@zepumph
Copy link
Member Author

zepumph commented Feb 21, 2023

I was not able to get lint running with the following commands:

cd rosetta
grunt lint
npm run lint

My current computer isn't running grunt lint-everything correctly, but that is a side issue. I believe that the lint-everything task is not a problem.

@zepumph zepumph removed their assignment Feb 21, 2023
@liammulh
Copy link
Member

@jbphet, can you reproduce this issue on your machine? I believe you and @zepumph use Windows.

@liammulh
Copy link
Member

I should say that I can't reproduce @zepumph's issue on my macOS machine.

@zepumph
Copy link
Member Author

zepumph commented Feb 24, 2023

It was on a borrowed machine that was mac. Ill do a bit of poking to see if I can reproduce on my main machine.

@zepumph zepumph assigned zepumph and unassigned jbphet and liammulh Feb 24, 2023
@zepumph
Copy link
Member Author

zepumph commented Mar 2, 2023

Hello! I was not a crazy person. It seems that files under the scripts directory aren't being linted.

When I apply this patch, I don't get an error with grunt lint or lint everything.

Subject: [PATCH] update lint to support mjs cjs, https://github.com/phetsims/rosetta/issues/370
---
Index: scripts/js/find-bad-pattern-translations.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scripts/js/find-bad-pattern-translations.js b/scripts/js/find-bad-pattern-translations.js
--- a/scripts/js/find-bad-pattern-translations.js	(revision 3846d1bc0f6212b12d2f0041b10d8e554a8ed0ed)
+++ b/scripts/js/find-bad-pattern-translations.js	(date 1677780002155)
@@ -23,6 +23,9 @@
 const templateVarRegex = /{\{?\w+}}?/g;
 const TRANSLATION_STRINGS_REPO_PATH = './babel';
 
+// TODO: Why doesn't this error?
+const x = '';
+
 // Define a simple assert function to help with debugging.
 const assert = ( condition, msg = 'assertion failed' ) => {
   if ( !condition ) {

Furthermore, I was confused how lint is running at all. I would have thought we needed a config at the top package.json:

Subject: [PATCH] update lint to support mjs cjs, https://github.com/phetsims/rosetta/issues/370
---
Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/package.json b/package.json
--- a/package.json	(revision 3846d1bc0f6212b12d2f0041b10d8e554a8ed0ed)
+++ b/package.json	(date 1677778932892)
@@ -19,5 +19,8 @@
     "eslint": "^8.34.0",
     "grunt": "^1.4.1",
     "winston": "^3.8.2"
+  },
+  "eslintConfig": {
+    "extends": "../chipper/eslint/node_eslintrc.js"
   }
 }

@zepumph zepumph assigned liammulh and unassigned zepumph Mar 2, 2023
zepumph added a commit that referenced this issue Mar 2, 2023
zepumph added a commit to phetsims/phet-info that referenced this issue Mar 2, 2023
zepumph added a commit to phetsims/chipper that referenced this issue Mar 2, 2023
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

3 participants