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 Makefile and fix some linting issues #293

Conversation

c0d1ngm0nk3y
Copy link
Contributor

fixes #260

Summary

This pr adds a Makefile and also fixes some linting errors so that make lint will run without error.

With the exception that some checks had been deactivated, because the errors were beyond the scope of this.

Use Cases

It's standard in a lot of Go projects. It also makes it easier to run test, lint, and CI.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@c0d1ngm0nk3y c0d1ngm0nk3y requested a review from a team as a code owner October 13, 2023 14:32
@c0d1ngm0nk3y c0d1ngm0nk3y added semver:patch A change requiring a patch version bump type:task A general task labels Oct 16, 2023
@c0d1ngm0nk3y
Copy link
Contributor Author

@paketo-buildpacks/java-maintainers The pr got a bit larger because of make make lint work. So there is some potential for conflicts. And new stuff could break the linting again. WDYT?

@c0d1ngm0nk3y
Copy link
Contributor Author

One question could be what about the failing linters commented out in golangci.yml. But I would argue, it is better to have this already in and then tackle the remaining linter errors with issues - maybe even one linter at a time.

Any thoughts?

@dmikusa
Copy link
Contributor

dmikusa commented Nov 10, 2023

One question could be what about the failing linters commented out in golangci.yml. But I would argue, it is better to have this already in and then tackle the remaining linter errors with issues - maybe even one linter at a time.

Any thoughts?

+1 for having it included. I know there are going to be issues we need to fix related to lints because we haven't been running them. Hopefully having it in there will help identify and fix them. If we could make it just WARN initially and not fail the build I think that would be good. Then we can correct them later & make it start failing the build so we don't introduce new lint issues.

@c0d1ngm0nk3y
Copy link
Contributor Author

@paketo-buildpacks/java-maintainers How to continue with this?

@dmikusa
Copy link
Contributor

dmikusa commented Dec 2, 2023

That's a good question. I think this is waiting on one of the @paketo-buildpacks/java-maintainers to review. I've been meaning to take a look but it keeps sliding down in my queue as other more urgent things pop up. I will try to review this though as it's important for v2. Thanks for your patience on this.

@c0d1ngm0nk3y
Copy link
Contributor Author

c0d1ngm0nk3y commented Dec 12, 2023

Thanks for your patience on this.

Maybe I can open an alternative pr to this that purely adds the Makefile. Commenting out all linters that currently fail. That should keep the pr much smaller. Once it is merged, on linter could be enabled after the other together with the fixes. Sound like I should have done it in the first place tbh. WDYT?

see #301

@c0d1ngm0nk3y
Copy link
Contributor Author

close in favour for #301 (easier to review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants