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

feat: adding golint and a Makefile #13

Closed
wants to merge 18 commits into from

Conversation

mfreeman451
Copy link

@mfreeman451 mfreeman451 commented Apr 18, 2023

  1. Added a Makefile that includes tests for golint, ineffassign, gosec, gocov, etc.
  2. Adding coverage.yml to ci for gocov
  3. Added golintci-lint.yml to ci for golint
  4. Updated a test so it would pass gosec check

.github/workflows/coverage.yml Show resolved Hide resolved
.github/workflows/golangci-lint.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go.work Outdated Show resolved Hide resolved
llms/openai/openaillm.go Outdated Show resolved Hide resolved
outputParsers/emptyParser.go Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Comment on lines +52 to +53
git add README.md
git commit -m "chore: Updated coverage badge."
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any better workaround or solution to such feature? I don't think it is elegant and proper to commit the README.md changes within CI/CD scope and interfere the branch that the developers working on, especially when we are doing some traditional GitFlow actions (such as rebase the upstream branch or so).

What do you think @tmc ?
If we are surely use such method to update the badges, I suggest to add a concurrency config in GitHub Actions workflow file to prevent conflicts, you can read more here: https://docs.github.com/en/actions/using-jobs/using-concurrency

Copy link
Contributor

@nekomeowww nekomeowww Apr 18, 2023

Choose a reason for hiding this comment

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

Or how about we make two workflow definitions:

  • Pull Request into main, focusing on testing and report the developers
  • Push into main, focusing on updating the test coverages badges

.gitignore Outdated
@@ -0,0 +1,6 @@
coverage.txt

This comment was marked as resolved.

- uses: actions/setup-go@v4
with:
go-version: '1.19'
cache: false

This comment was marked as resolved.

mfreeman451 and others added 5 commits April 18, 2023 09:50
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
@tmc
Copy link
Owner

tmc commented Apr 18, 2023

I really like what I'm seeing here, thanks for reviewing @nekomeowww -- good input.

mfreeman451 and others added 2 commits April 18, 2023 21:54
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
@nekomeowww
Copy link
Contributor

There are some conflicts. Remember to get your branch updated 😉

@rdbell rdbell mentioned this pull request Apr 19, 2023
@rdbell
Copy link
Contributor

rdbell commented Apr 19, 2023

Looking forward to a more uniform codebase with conventions to follow. Let's get this in.

uses: ad-m/github-push-action@master
with:
github_token: ${{ github.token }}
branch: ${{ github.head_ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Every file should ends with a empty new line.

Suggested change
branch: ${{ github.head_ref }}
branch: ${{ github.head_ref }}

You can ask IDE/editors perform such actions automatically, if you are using Visual Studio Code, you could add one line that enable this feature easily:

{
  "files.insertFinalNewline": true,
}

Comment on lines +52 to +53


Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please format the new lines of end of the file?

Suggested change

@tmc
Copy link
Owner

tmc commented Apr 21, 2023

I believe this is obviated by #42

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