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

Enhance linting #95

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Enhance linting #95

merged 5 commits into from
Feb 14, 2024

Conversation

dkarnutsch
Copy link
Contributor

Following problems are addressed here:

  • Lint-Staged should match actual lint config
  • Linting project root did not take into account subfolders (i.e. .gitlab ci) due to misconfigured glob pattern
  • Yaml files did not get linted (e.g. helm files) due to missing extension
  • Files outside of folder "src" from microservices (api/admin/site) did not get linted as eslint did only get executed on "src" folder (e.g. next-config or readme)
  • Harmonize lint calls with run-p

However, this has bad IDE support as we have a prettierignore with (api/admin/site/create-app). This can be resolved by either copying the .gitignore into .prettierignore and removing the ignore patterns or by updating to prettier 3 (which ignores all files from .gitignore by default, https://prettier.io/blog/2023/07/05/3.0.0.html#cli)

@dkarnutsch
Copy link
Contributor Author

Failing pipeline is addressed with vivid-planet/comet#1540

@johnnyomair
Copy link
Collaborator

Failing pipeline is addressed with vivid-planet/comet#1540

I'd prefer ignoring block-meta.json over having Prettier as a dependency in @comet/cms-api. While it doesn't match Prettier's standards, the file still is formatted.

@dkarnutsch
Copy link
Contributor Author

dkarnutsch commented Jan 15, 2024

Failing pipeline is addressed with vivid-planet/comet#1540

I'd prefer ignoring block-meta.json over having Prettier as a dependency in @comet/cms-api. While it doesn't match Prettier's standards, the file still is formatted.

Done with 85de1c1

I had to copy .gitignore into .prettierignore because prettier is yet to support multiple ignore files (prettier/prettier#8048)

@dkarnutsch
Copy link
Contributor Author

Overall I'm not 100% happy with the solution :( but it is better than before, because we did e.g. not lint CI/CD files

IDE support is not great when ignoring all microservices and the difference between CLI and IDE (for ignoring files) is far from ideal

@johnnyomair
Copy link
Collaborator

Overall I'm not 100% happy with the solution :( but it is better than before, because we did e.g. not lint CI/CD files

Why?

However, this has bad IDE support as we have a prettierignore with (api/admin/site/create-app). This can be resolved by either copying the .gitignore into .prettierignore and removing the ignore patterns or by updating to prettier 3 (which ignores all files from .gitignore by default, https://prettier.io/blog/2023/07/05/3.0.0.html#cli)

I'd say we copy all existing .gitignore into .prettierignore and add a comment to remove them once we upgrade to Prettier v3

@johnnyomair johnnyomair removed their request for review January 18, 2024 14:51
@dkarnutsch
Copy link
Contributor Author

I'd say we copy all existing .gitignore into .prettierignore and add a comment to remove them once we upgrade to Prettier v3

Done

@dkarnutsch
Copy link
Contributor Author

@kaufmo please review

@dkarnutsch
Copy link
Contributor Author

@fraxachun @nsams please review

@dkarnutsch dkarnutsch merged commit 5f76a4b into main Feb 14, 2024
2 checks passed
@dkarnutsch dkarnutsch deleted the enhance-linting branch February 14, 2024 10:18
thomasdax98 pushed a commit that referenced this pull request Feb 28, 2024
#95 (comment)

Fixes the "bad IDE support" by allowing allowing prettier in IDE for
api/admin/site/create-app but executing the cli-check only for
root-files.
johnnyomair added a commit to vivid-planet/comet that referenced this pull request Jul 22, 2024
Adopt enhancements from
vivid-planet/comet-starter#95 for core.
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

Successfully merging this pull request may close these issues.

4 participants