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

Implement fixer #36

Merged
merged 5 commits into from Feb 15, 2024
Merged

Implement fixer #36

merged 5 commits into from Feb 15, 2024

Conversation

firelizzard18
Copy link
Contributor

@firelizzard18 firelizzard18 commented Jan 17, 2024

Updates the analyzer to support automatic fixes for golangci-lint.

Updates #2.

@firelizzard18
Copy link
Contributor Author

@denis-tingaikin

@denis-tingaikin
Copy link
Owner

@firelizzard18 Woot! Looks good. Let me test it a bit.

@denis-tingaikin
Copy link
Owner

denis-tingaikin commented Feb 13, 2024

@firelizzard18 It also seems like unit tests detect SIGSEGV. https://github.com/denis-tingaikin/go-header/actions/runs/7561279933/job/21541496704?pr=36

Could you have a look? 

I've added an assignee to myself to get notifications; feel free to ping me inside PR when it is fixed.

@firelizzard18
Copy link
Contributor Author

firelizzard18 commented Feb 13, 2024

@denis-tingaikin I fixed the panic. I'm not sure why I had written if i.Message() == "", that clearly was not the right condition...

TestAnalyzer_YearRangeValue_ShouldWorkWithComplexVariables is failing on main with the same error, so I don't think that's something I broke. I also added TestFix to verify that a fix is actually produced and produced in the correct way. I have manually verified that my changes work with golangci-lint, and work for all the cases I could think of in terms of how the header comment is structured.

@denis-tingaikin
Copy link
Owner

As I can see we have only one problem with a test.

It's related to oudated input for the expected value here https://github.com/denis-tingaikin/go-header/blob/main/analyzer_test.go#L57

that we can fix with something like this:

---require.Nil(t, a.Analyze(header(`A 2000-2022 B`)))
+++require.Nil(t, a.Analyze(header(fmt.Sprintf("A 2000-%v B", time.Now().Year()))))

@firelizzard18
Copy link
Contributor Author

@denis-tingaikin I applied the fix for the test

@denis-tingaikin
Copy link
Owner

@firelizzard18 Great work! Merging..

@denis-tingaikin denis-tingaikin merged commit 171fdb2 into denis-tingaikin:main Feb 15, 2024
1 check failed
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.

None yet

2 participants