Skip to content

Conversation

@spencerschrock
Copy link

@spencerschrock spencerschrock commented Nov 29, 2023

Saw a few occurrences of this in my code, and noticed the linter wasn't picking them up.

r := strings.NewReader(string(content))

MIRROR_FUNCS.md was edited manually, but the tests were generated with make test-generate.

@butuzov
Copy link
Owner

butuzov commented Dec 1, 2023

I like it. Let me check what Ican do about having MIRROR_FUNCS.md generated and i will merge you PR.

Thank you!

@butuzov
Copy link
Owner

butuzov commented Dec 4, 2023

@spencerschrock thank you for yout PR, but for now lets put it onhold. While i would go for using correct struct/function both functions will return different types (yeah, same interface but types are different). So it require a bit different implementation. I will work with your code and no change required from your side.

However it most probably will take some time on my side, as I am now not in friendly conditions of home office.

cheers.

@butuzov butuzov self-assigned this Dec 4, 2023
@spencerschrock
Copy link
Author

While i would go for using correct struct/function both functions will return different types (yeah, same interface but types are different). So it require a bit different implementation. I will work with your code and no change required from your side.

Ah shoot, I glanced over that. I just saw the Reader type and forgot one is strings.Reader and one is bytes.Reader. I can see how this would be problematic depending on how the resulting type is assigned/used.

This sort of problem has prevented me from removing other string/[]byte conversions in my code. For example, where I'm unable to change to r.Find(line) below because I need the result to be a string vs []byte.

var found string = r.FindString(string(line))

I'm new to writing/patching static analysis tools, so I'm excited to see your future changes.

However it most probably will take some time on my side, as I am now not in friendly conditions of home office.

Of course, take as long as you need. Thanks for taking a look at the PR in the first place, and of course stay safe.

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.

2 participants