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

[Task] Apply yarn format:apply to ./src/ #740

Closed
2 of 5 tasks
maxcao13 opened this issue Dec 9, 2022 · 2 comments · Fixed by #780
Closed
2 of 5 tasks

[Task] Apply yarn format:apply to ./src/ #740

maxcao13 opened this issue Dec 9, 2022 · 2 comments · Fixed by #780
Assignees
Labels
chore Refactor, rename, cleanup, etc.

Comments

@maxcao13
Copy link
Member

maxcao13 commented Dec 9, 2022

Dependent on #753
Dependent on #747

There are around 500 fixes that can be applied automatically with the new eslint plugin import/order.
Unfortunately it seems import/first has to be disabled because of problems with mocking the Date system time, which needs to be mocked before imports in the DateTimePicker.test.tsx.

Then yarn eslint:apply can be safely applied to every PR before pushing.

There is additional task of fixing ALL eslint warnings manually so that the CI can start to check for it. That might be another chore if it sounds like a good idea.

I will prefer to open a PR when there are not as many open right now so that rebases aren't painful.

Tasks

  • Apply yarn format:apply to automatically fix fixable eslint issues
  • Manually go through all eslint warnings to fix them
  • Re-enable yarn eslint within ci.yaml as part of the CI automation process
@maxcao13 maxcao13 added chore Refactor, rename, cleanup, etc. needs-triage Needs thorough attention from code reviewers labels Dec 9, 2022
@maxcao13 maxcao13 self-assigned this Dec 9, 2022
@maxcao13 maxcao13 moved this to Todo in 2.3.0 release Dec 9, 2022
@andrewazores
Copy link
Member

Then yarn eslint:apply can be safely applied to every PR before pushing. There is additional task of fixing ALL eslint warnings manually so that the CI can start to check for it. That might be another chore if it sounds like a good idea.

#208

I had the intent to do that quite a while ago, and obviously at that time I also just gave up on it. But, I think the codebase now is actually in overall better shape, and we have a lot more automated tooling that should help. So perhaps it's an easier task despite the fact that there is a lot more code now.

@maxcao13
Copy link
Member Author

maxcao13 commented Dec 9, 2022

Oh, I see. That's why the run: yarn lint in the ci.yaml has been disabled.

I do think it is not too hard for our codebase to be cleaned up for the closed task. Okay, I will add an additional subtask to this task to manually fix the warnings. Shouldn't be too painful I think. I will just start after those PRs are merged to save extra work.

@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Dec 13, 2022
@github-project-automation github-project-automation bot moved this from Todo to Done in 2.3.0 release Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants