-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
Revised attempt to audit code #425
base: release/1.3
Are you sure you want to change the base?
Conversation
One thing I should apologize for: there was an automated change to get rid of "new " in favor of just "new"; the spacing rules in .editorconfig are vague on this. The changes to new() do make sense; otherwise, it's some extra cruft you probably didn't wanna sort through. |
Thanks, I am going through them, but will probably take a while. Most are good, things I welcome but never gotten to due to the sheer amount of files. I also am not a big fan of inline LINQ, I mean the for via keywords. Sometimes I prefer the extensions, sometimes I rather have it with a normal for each / loop, as debugging LINQ is not easy. What also needs to be noted, is that LINQ is usually much worse performance as plain for loops. So I need to think about those changes a bit. |
Understood. I think we can revert some of the syntax changes via an
.editorconfig preference & a re-scan; I don't remember off-hand if there
was already one in place. There seems to be love-hate with LINQ:
anything I read on it sings its praises; but you're not the first one to
mention that it might have a significant performance issue.
Thanks for taking a look at this. Hope all is well.
Michael Adams
https://unquietwiki.com/ <http://www.unquietwiki.com/>
------ Original Message ------
From "Robin Krom" ***@***.***>
To "greenshot/greenshot" ***@***.***>
Cc "Michael Adams" ***@***.***>; "Author"
***@***.***>
Date 7/30/2022 12:40:38 PM
Subject Re: [greenshot/greenshot] Revised attempt to audit code (PR
#425)
Thanks, I am going through them, but will probably take a while.
I need to check every file, as I really prefer to see what the changes
are.
Most are good, things I welcome but never gotten to due to the sheer
amount of files.
But some got this "we just change everything because resharper or
whatever tool is used, says so" feeling.
I do not always like the ternary syntax, that is the ? : notation.
Because things do not get better readable, at least there are quite a
few cases that this suffers.
I also am not a big fan of inline LINQ, I mean the for via keywords.
Sometimes I prefer the extensions, sometimes I rather have it with a
normal for each / loop, as debugging LINQ is not easy. What also needs
to be noted, is that LINQ is usually much worse performance as plain
for loops. So I need to think about those changes a bit.
—
Reply to this email directly, view it on GitHub
<#425 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHV7P6WJYWOZE5K6AMQCQ3VWWALNANCNFSM53TT5LSQ>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
… |
Hey @Lakritzator : hope the past few months have found you well. So I finally found some time to look at this again; being less aggressive on changes & avoiding the runtime upgrade. I know back then you were worried about Office plugin support; I think that should work, since I didn't touch the Dispose patterns. One thing that came up in my testing: it looks like there are missing OAuth hooks / allowances for uploading to Google, Imgur, etc; maybe there's something I didn't set up beforehand, but thought I'd mention it. I also didn't mess with the InnoSetup on this; currently just running this as Debug build for now.