-
Notifications
You must be signed in to change notification settings - Fork 122
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
Fix/270 fix eslint config #271
Conversation
April-Bates-Dev
commented
Sep 12, 2023
- Editing CI to include linting
- Fixing linting errors
- Fixing broken linting config
e3c80c9
to
b245340
Compare
Test results summary:✅ [PASS] - Test case: Bad variable name SUMMARY: ✅ PASS: 2 - Tests Powered by Code Review GPT |
32f3040
to
8058038
Compare
6ccf610
to
bf9e538
Compare
389d864
to
9492b00
Compare
7994bc8
to
2ed27b2
Compare
2ed27b2
to
93a1610
Compare
@@ -39,8 +39,6 @@ const formatTestResult = (result: testResult, message: string): string => { | |||
return chalk.yellow(`⚠️ [WARN] - ${message}`); | |||
case testResult.FAIL: | |||
return chalk.red(`❌ [FAIL] - ${message}`); | |||
default: | |||
throw new Error(`Unknown test result: ${result}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Note] Removed because it's an impossible case- there are only 3 options within the enum- other options are impossible and it threw a linting error
ad0a2e2
to
4e64014
Compare
4e64014
to
78c0d21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a master file and then each sub file inherits from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we were gonna have a shared file like that we are gonna have monorepo issues 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally am in favour of fixing this when we have the monorepo setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it to the monorepo ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I see the point in changing this to make it much more complicated if it still doesnt work? I think I would prefer if it failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm- I see what you are saying, but it if is reverted we have undone one fix (the folders issue). I think it is better to leave it in a state that we know is closer to working so someone doesn't end up solving the same problem again.