Comments (6)
contextcheck
is #1911 and let's addcontainedctx
there as well, it's essentially the same problem for usfunlen
andgocognit
(orgocyclo
which is very similar) are a bit too vague for me, they're nice, but they can make you work just to please some linter and not really improving anythingasciicheck
can be enableddogsled
issues in many cases just can't be fixed, if you have a function returning 4 variables and you only need one, what are you going to do?exhaustruct
is likely to lead to a lot of false positives, in many cases I'd prefer more concise code and ability to extend structure without touching every line initializing itexportloopref
is enabled in NeoGo, should be enabled here as wellforcetypeassert
don't seem to be useful to me, if it's written this way then it's OK to panic heregoconst
andgomnd
can be tried, but I fear there will be false positivesnilerr
can be triedreassign
is enabled in NeoGounconvert
should be OK
Linters are great, but in many cases it's a balance between false positives/time to spend pleasing the linter and the effect of the problem it can prevent. At the same time looking at your list I see some new ones, so some periodic reshuffling of the linter set can be useful.
from neofs-node.
funlen and gocognit (or gocyclo which is very similar) are a bit too vague for me, they're nice, but they can make you work just to please some linter and not really improving anything
Mostly always I would say that 500 lines long functions are strange. If it is a huge switch
or some other exceptions that really can't be done any other way, a few nolint
s in a 100K lines code base is OK to me.
if you have a function returning 4 variables and you only need one, what are you going to do?
Ask yourself what led you to the times you use func that returns N args and you do not need most of them (if the func is the internal one, of course, external libs can't be fixed but I do not think we use such).
exhaustruct is likely to lead to a lot of false positives
What do you call "false positives" in that context? My arg is the example I provided: panic in the master
that was hard to predict on review.
forcetypeassert don't seem to be useful to me, if it's written this way then it's OK to panic here
I would force a dev to check asserting and panic explicitly, it (may) adds details, it saves reviewer's time, it does not allow mistakes when a dev writes asserting to check his code but forget to add a check (where it is needed, of course). Anyway, if panicing is OK in some scenarios, we usually write comment about that (~the same number of chars compared to panicing with some detailed message).
goconst and gomnd can be tried, but I fear there will be false positives
What do you call "false positives" in that context?
Linters are great, but in many cases it's a balance between false positives/time to spend pleasing the linter and the effect of the problem it can prevent.
Sure, that is why I filtered them, and not just copy-pasted all the linters golangci-lint
allows. Almost every linter I suggested would have saved us from issuing, testing, fixing, and reviewing some stupid bugs. What takes more time? Well, that is an open discussion.
from neofs-node.
dogsled
Ask yourself what led you to the times you use func that returns N args and you do not need most of them
But other users may use all of them anyway, so you'll waste some time, but gain nothing. It can be tried though, if there is a small number of places like this and all of them can be legitimately fixed, then maybe it's OK.
BTW, we have exactly one case of this (but tests: false
, meh).
exhaustruct
What do you call "false positives" in that context?
pkg/network/address.go:54:11 exhaustruct Opaque, User, Path, RawPath, OmitHost, ForceQuery, RawQuery, Fragment, RawFragment are missing in URL
...
pkg/morph/client/notary.go:563:11 exhaustruct Rules is missing in Signer
pkg/morph/client/notary.go:575:12 exhaustruct Rules is missing in Signer
...
pkg/innerring/processors/netmap/process_peers.go:67:10 exhaustruct InvokePrmOptional is missing in AddPeerPrm
pkg/innerring/processors/netmap/process_peers.go:169:11 exhaustruct InvokePrmOptional is missing in UpdatePeerPrm
forcetypeassert
I would force a dev to check asserting and panic explicitly, it (may) adds details
What detail can I add to https://github.com/nspcc-dev/neo-go/blob/9185820289ce942153bc5ba012e677b5278f0cb5/cli/server/server.go#L469? "If you see this panic, then whoever wrote this code is an idiot, but this is not likely to help you anyway, the system has failed"?
It's not always the case, but I'd expect the majority of them to be like that.
goconst and gomnd
What do you call "false positives" in that context?
pkg/morph/client/notary.go:417:35 gomnd mnd: Magic number: 2, in <argument> detected
pkg/morph/client/notary.go:552:35 gomnd mnd: Magic number: 2, in <argument> detected
...
pkg/util/os.go:9:32 gomnd mnd: Magic number: 0110, in <argument> detected
...
pkg/innerring/processors/settlement/audit/calculate.go:45:27 gomnd mnd: Magic number: 30, in <argument> detected
from neofs-node.
dogsled
But other users may use all of them anyway, so you'll waste some time, but gain nothing.
I meant if it is a private func for internals (like it is in my example), then you (may) want to split it into subfuncs and not do _, _, _, val := func()
.
exhaustruct
pkg/network/address.go:54:11
Nothing can be done, agree.
pkg/morph/client/notary.go:563:11
Do you mind Rules: nil
? It would be ok for me.
pkg/innerring/processors/netmap/process_peers.go:67:10
Bad design, IMO. And the linter would highlight it. We are gonna remove it, aren't, we?
forcetypeassert
What detail can I add to https://github.com/nspcc-dev/neo-go/blob/9185820289ce942153bc5ba012e677b5278f0cb5/cli/server/server.go#L469? "If you see this panic, then whoever wrote this code is an idiot, but this is not likely to help you anyway, the system has failed"?
I am not sure about neo-go code. My ide says that GetStateModule
always returns *corestate.Module
and that is the only (exported) implementation of the StateRoot
interface, so I can't say why it is done that way. I would add details that explain why that cast is necessary and why it should not be changed when another dev comes here and adds one more implementation. (I may be such a dev if I have 2 mins on fixing smth in that repo).
Also, it may add and may not add any details. As I already said, usually we add comments anyway, your example is not an exception.
goconst and gomnd
pkg/morph/client/notary.go:417:35
pkg/morph/client/notary.go:552:35
Well, they are real examples of the magics, no? We add comments to explain what is happening here. This is not smth obvious (even for a neofs dev payments and order details are tricky). Also, we reuse them (you added two links and both of them are the same numbers meaning the same).
pkg/util/os.go:9:32
Arguable, but agree that const for that may be too much (but having it does not bother me).
pkg/innerring/processors/settlement/audit/calculate.go:45:27
Magic, no? Sometimes we add comments (that also can be comments to some well-named constants) to such numbers.
from neofs-node.
you (may) want to split it into subfuncs
Do we have an issue for our single example of this behavior? I'd leave it to that. Maybe this code is bad, maybe it can be improved, but I'd not add a linter for it.
Do you mind Rules: nil?
Yes, I do. I want some specific signer, this signer doesn't use Rules, so I don't want to think about many other fields that can be set. For some simple signers like None or CalledByEntry I also don't care about AllowedContracts
or AllowedGroups
(they won't work anyway), so I don't want to be setting them.
Now the problem would be if I'm to set CustomContracts
and not specify AllowedContracts
. It'd be a bug. But I can set it to nil
as well and it'd also be a bug.
when another dev comes here and adds one more implementation
He won't. There is no need to, other than tests of some kind.
Well, they are real examples of the magics, no?
We have exactly this number of signers and all the code around works with exactly that number. You can't change a constant and expect all the other code to work, for example. Compare that to defaultNotaryValidTime
, that's a nice constant that I can change independently of any other code. So 2/3 constants don't bring a lot of value (while they can make some things more readable), but you'll have to manage them in your code and there is some cost to it.
pkg/innerring/processors/settlement/audit/calculate.go:45:27
Magic, no?
Well, it's a GB, as the variable name suggests. We can write 1024*1024*1024
here, but then gomnd
will ask us to have some constant for 1024. That's exactly the thing I'm talking about --- very soon you start spending time and effort just to have some positive feedback from the linting machine instead of solving real problems. And these examples were taken randomly (I just don't want to spend time digging into all of them), some will require more time and effort than others. Some of course will be seriously improved (goconst
warnings (there are just three of them) looked fair to me, for example), but for a linter to be enabled by default it must have some serious SNR, these ones just don't have it.
from neofs-node.
- dupword: sounds like a joke, but I have fixed so many duplicated comment words in that repo (and I just found one more example right now) so it is not that fun to me now. TBH, It is also strange to me, my IDE shouts at me when it sees duplications, but I can understand that not every editor does the same.
from neofs-node.
Related Issues (20)
- panic: missing context
- `neofs-adm` can not generate SN config for testnet
- Adapt Neo account contract groups HOT 1
- Evacuate test fails sometimes
- Refactor IR HOT 2
- config.yml is outdated HOT 3
- Change Notary request senders and try to deduplicate main transactions in auto-deploy procedure HOT 3
- Eco balance in auto-deploy procedure HOT 2
- Who deploys contracts in auto-deploy procedure? HOT 5
- Revise log messages of Sidechain auto-deployment procedure HOT 4
- missing port in address HOT 1
- StartWhenSynchronized hardcode blocks new network start HOT 5
- drop az-buky-vedi dependency
- IR: Revise config and make it more convenient for administration
- IR: `morph.consensus.peers.min` config defaults incorrectly
- batch storage wallets generation HOT 5
- Revise blockchain height check on startup HOT 3
- cli: Improve UX concerning attributes' setting in `object put` command HOT 1
- metrics should have the highest priority HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from neofs-node.