Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.5k
Addgofactory linter#4196
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Addgofactory linter#4196
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Hey, thank you for opening your first Pull Request ! |
CLAassistant commentedNov 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
ldez commentedNov 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.
Pull Request Description
Linter
The Linter Tests Inside Golangci-lint
|
Uh oh!
There was an error while loading.Please reload this page.
Maybe to combine with check from#3488? |
I don't want to combine |
2. Added tests3. Added config4. Added linter
Uh oh!
There was an error while loading.Please reload this page.
Is there anything else that I can do to get the PR be approved and merged? |
Antonboom left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the linter!
In addition to review comments:
TestdataDir()->analysistest.TestData()- Case like
typeOtherStructstruct {}typeStructstruct {OtherOtherStruct }funcNewStruct()Struct {returnStruct{Other:OtherStruct{},// warning?}}
Type assertion, type declaration and type underlying,tests <- Broken link
- Struct with type params?
typeStruct[Tany]struct {// type params could be many (this is different ast node)}_=Struct[T]{}// Warning
- Import alias (and affect on report warning?).
- You covered slice, but not map, channel, func call, etc. places where obj could be constructed.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
2. Added test with options
Uh oh!
There was an error while loading.Please reload this page.
#golangci/golangci-lint#41961. TestdataDir() -> analysistest.TestData()2. Described False-Negative cases as testcases and in README.md3. Fixed link for type_nested.go.skip in README.md4. Added testcase with generic5. Added testcase with alias6. Added testcase with map, chan, array, func#golangci/golangci-lint#41967. Added glob syntax for pkgs8. Renamed options9. Extended unexpected code message10. Added unimplemented testcases
maranqz commentedNov 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Could I resolve the case on Winter Weekends, not now?)
|
Just a reminder for the maintainers: before reviewing the code, the checklist should be handled. |
ldez commentedNov 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The use of the tests dedicated to linters that need dependencies is not the right way. I'm not sure about the pertinence of this linter because Go doesn't require constructors, and data structure is a good pattern, I think this will lead to a lot of false positives. |
maranqz commentedNov 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Do you mean tests should be like this and remove subdirectories? package gofactoryimport"net/http"funclocal() {_= http.Request{}// want..._,_=http.NewRequest("GET","http://example.com",nil)} |
My motivation was to create the linter for the domain layer with business logic in the factories, which cannot be skipped and where any structures should always be valid. I agree that using the go-factory-lint in any place is not a good idea, so I added options Should I force set |
Uh oh!
There was an error while loading.Please reload this page.
2. Removed nested directory in test
maranqz commentedNov 26, 2023 • edited by ldez
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ldez
Uh oh!
There was an error while loading.Please reload this page.
I removed subdirectories in tests. I'm not sure what you mean. I can't because the linter needs this to determine whether it is a function or a type when we cast types like this. package castingimport ("factory/unimplemented/casting/nested")funcToNestedMyInt() {_=nested.MyInt(1)// want `Use factory for nested.Struct`} |
Uh oh!
There was an error while loading.Please reload this page.
2. renamed options3. changed linter version
| "--enable-all", | ||
| "-D", | ||
| "nosnakecase,gci", | ||
| "nosnakecase,gci,gofactory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I hope that is correct solution to pass tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
your tests are not in the right places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you mean I should movetest/testdata/gofactory/blocked.go totest/testdata/gofactory/gofactory_package_globs_only.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no, your tests must be insidetest/testdata/ and not a dedicated folder.
and your linter should be removed fromtest/linters_test.go.
Uh oh!
There was an error while loading.Please reload this page.
| funcTestUnsafeOk(t*testing.T) { | ||
| testshared.NewRunnerBuilder(t). | ||
| WithNoConfig(). | ||
| WithArgs("--enable-all"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how does this change relate to PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I return, can I add-D gofactory to fix linter errorUse factory for unsafe.Pointer.
Or is solution from comment is better?
#4196 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should be reverted.
| "--enable-all", | ||
| "-D", | ||
| "nosnakecase,gci", | ||
| "nosnakecase,gci,gofactory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
need to rollback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added because of linter errorUse factory for unsafe.Pointer.
Should I add option to exclude checking of std libraries in gofactory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should I add option to exclude checking of std libraries in gofactory?
Nice idea, but I think in case ofexclude-std=false we will receive false positive anyway, because a lot of std types could be (or must be) used without factory by design. Likeunsafe.Pointer orsync.Mutex.
Zero value is useful 🙂
Look at
$ cd $GOROOT/src$ gofactory -json ./... | jq '.[] | .[] | .[] .message' | sort | uniq| funccall(_ http.Request) {} | ||
| funcalias() { | ||
| _= alias_blocked.Request{}// want `Use factory for http.Request` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cosmetic question: why notUse factory for alias_blocked.Request?
at fist time I was confused, but cannot decide what option is better 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Idk too)
Could you decide how to better based on your experience?
I checkedwrapcheck,go-reassign. They both used different printing way for package names.
I don't find another linter which uses package names.
wrapcheck:error returned from external package is unwrapped: sig: func encoding/json.Marshal(v any) ([]byte, error)
go-reassign:reassigning variable EOF in other package alias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO, the option that is better for grep and common understanding – to keep the source code and warnings for it consistent:
_= neturl.URL{}// want `Use factory for neturl.URL`_=&url.URL{}// want `Use factory for url.URL`
Uh oh!
There was an error while loading.Please reload this page.
Thelinter checks that the structures are created by the factory, and not directly.
The checking helps to provide invariants without exclusion and helps avoid creating an invalid object.
related to#2708