echo icon indicating copy to clipboard operation
echo copied to clipboard

remove unused param

Open harrisonho99 opened this issue 2 years ago • 1 comments

remove unused param

harrisonho99 avatar Jul 23 '22 09:07 harrisonho99

I think this commit it can be good for memory.

Kamandlou avatar Aug 22 '22 18:08 Kamandlou

I think this commit it can be good for memory. I don't think memory for tests run only locally by the developer is an issue.

@harrisonho99 Why did you even come across that. Did you see a warning in your IDE? I see no real value in changing it, unless it silences a warning. From a developers perspective I like the similar signatures of the testing methods.

lammel avatar Nov 30 '22 12:11 lammel

I have not checked what assembler differences are but I doubt that there is. If I recall correctly parameters are used by their indexes in assembly. Go compiler can output assembly and there are nice tools to give you visual reference https://github.com/loov/lensm

I know that IntelliJ IDEA/GoLand shows unused parameters as grey and lists them under "Problems -> Go -> Unused parameters". I do not know any static analysis tool warning about these.

If we would accept this PR we probably should "fix" all places where similar situation exists. From IDEA report I know there are more than one in repo. As author have not done that - I assume intention is probably more related to Github Achievements.

Sorry for being negative but these ad-hoc/random partial fixes to tests starte after Github rolled out achievements in late 2021.

Improving tests is not a bad thing but if you know that there are more place with similar problem and choosing not to fix them is a problem as those places are potentially next N amount of PRs that need to handled.

aldas avatar Nov 30 '22 13:11 aldas

No further comments, so closing.

lammel avatar Feb 21 '23 21:02 lammel