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

Add Codacy and Friends Configuration #111

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Conversation

confused-Techie
Copy link
Member

This PR adds a Codacy and ESlint config.

ESlint is included within Codacy, so we want to configure them really at the same time to ensure things work how we expect.

Much of the config of both has been taken directly from the Pulsar config, to ensure a relatively seamless flow between the two. This configuration will still keep Codacy out of the way, only reporting on the PR a score (Although this PR makes Codacy more out of the way than now, by fine tuning that score to be even less aggressive.)

Additionally, we are fine tuning some of the enabled rules within ESlint to ensure we only get alerts about the things we care about, where I've gone ahead and disabled the overzealous or especially opinionated rules that we have alerts for.

@DeeDeeG
Copy link
Member

DeeDeeG commented Dec 15, 2023

Are the "commented"-looking settings in .codacy.yaml done on the web dashboard for the Codacy account? Or does codacy read through those comments in the yaml file and act on them?

But if this makes Codacy a little less noisy, then I can "rubber stamp" approve this. Hoping for some clarification on the .codacy.yaml file, since I interact with this repo a lot I suppose it's good for me to understand what the file does before I approve.

I've seen a .eslintrc before, so I get the gist of what that's doing.

@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks for taking a look.

But yeah those comments are truly just comments. Basically there's so many things to configure within Codacy, but their config file is underwhelming in features. So I did this over in Pulsar, where the other important bits are added as a comment. They mean nothing technically, but allow us to get on the same page, and help prevent to many big changes within any kind of PR, to help ensure once we have it working how we want it, that it doesn't get fiddled with more.

So those comments are much more notes of the ideal configuration. And if the PR was merged, then PPM's config would be changed within the web ui to match those comments.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the response / clarification!

I'll approve so we can get this added.

If any tweaks are needed when subsequent PRs make actual code changes, and we can see how Codacy reacts to those, then that's good and gives a full enough picture to act on.

that's somewhat of a chicken or egg thing, can't see if this works perfectly with this repo without the config in place for actual code change PRs. That said, since this is based on what works at core repo, should be a great start and maybe not need any changes after all.

So I'm not familiar with exactly how these rules got derived, I am review+ ing this based on trusting it's working well elsewhere.

tl;dr 👍

@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks!

Yeah you are totally right with the chicken or egg thing.

But as for the current ruleset, it basically comes from some rules that we break often in this and other repos, that are flagged by some programs in Codacy but in the end are nearly or purely stylistic choices. Such as using " or '. It doesn't really make a difference, and while I have a preference, we have never agreed to any code standards, so it doesn't make sense to flag something just because it's some programs default, I'd rather disable the rule until we determine our own code standards.

But I'll g ahead and merge this, then apply all of the new settings. From there the next PR or commit should be compared against those standards

@confused-Techie confused-Techie merged commit 32a2833 into master Dec 15, 2023
11 checks passed
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.

2 participants