-
Notifications
You must be signed in to change notification settings - Fork 22
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: lint config package #3382
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: schogges <[email protected]>
Signed-off-by: schogges <[email protected]>
Signed-off-by: schogges <[email protected]>
Signed-off-by: schogges <[email protected]>
Signed-off-by: schogges <[email protected]>
✅ Deploy Preview for kuma-gui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: schogges <[email protected]>
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.
Nice looks/sounds great 👍
I left one q, but lemme just give it whirl also, brb
mk/index.mk
Outdated
.install/clean/workspace: | ||
@cd $(FILE_DIR).. && \ | ||
echo "Recursively removing all node_modules/ directories in `pwd`..."; \ | ||
$(MAKE) .clean |
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.
Don't we already have clean target somewhere? If so, could that be reused somehow?
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.
Yes, there is the one I trigger with $(MAKE) .clean
. Would you prefer to use that directly in the root Makefile and remove this index file?
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 so 🤔 , then there's no other reason to have this file, is that right? Guessing using that other one works as expected?
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.
Yes, we could remove that file then 👍
Flake: https://github.com/kumahq/kuma-gui/actions/runs/12711384283/job/35434626365?pr=3382 Restarting... |
Signed-off-by: schogges <[email protected]>
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.
Sorry for the wait, had to jump on a couple of other bits.
I tried it locally and I think this looks good to me, I left 2 more q's, lemme know on those and then I think we can 👍 this!
|
||
.PHONY: clean | ||
clean: | ||
@echo "Recursively removing all node_modules/ directories in `pwd`..."; \ |
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.
If we are adding this echo
should it be in the implementation of .clean
so it does the same everywhere?
@@ -0,0 +1,3 @@ | |||
{ | |||
"*.{cjs,js,ts}": "make lint" | |||
} |
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 wonder if there is anyway to move this to the workspace root and have it apply to the entire workspace? Doing something on the repo before staging applies to the entire workspace, so is probably more workspace configuration rather than individual project configuration, wdyt?
We might also want to move these to JS files: https://github.com/lint-staged/lint-staged?tab=readme-ov-file#using-js-configuration-files so we can have a config in @kumahq/config
and then import that from each repo. Up to you whether you want to try this bit as a follow up PR at some point or here. I think this would help when we come to implement lint-staged elsewhere also, we'd just import this config from @kumahq/config
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.
P.S. If any of this ☝️ is in the slightest bit problematic, lets leave it for a follow up
The main goal of this change is to add linting for the
config
package. While doing this I wanted to take the chance and add a few more things:Makefile
for root operationsinstall
andclean
for easy cleaning and reinstallation of dependenciesimport/no-extraneous-dependencies
to error in case we make use of a module without installing it beforehand. This caused some issues already and a first fix was added with chore(config/deps): explicitely install globals, @eslint/js #3381. But also here I have to installglob
as it wasn't listed in the config's dependency list, but is imported in packages/config/scripts/ci.cjs#L1.packages/config