Closed Adrijaned closed 5 years ago
Adrijaned
commented
5 years ago Some more small improvements, added code folding with preprocessor #if blocks as described in #139 . Seems to be throwing IDE exception on my machine, but the build environment seems quite funky for me in general - not sure if it really is an issue, and if so, no idea what to do to fix it - this is my first time ever looking into JB plugin dev environment after all. :czech_republic:
Darkyenus
commented
5 years ago Thank you for the PR and your time.
I agree that the formatting is often funky, but this PR isn't always a clear improvement (there are some places, where one-line ifs are more readable than exploded versions, I have seen some elses on a new line, etc.). Additionally, these mass reformats mess up git history and blame, so I prefer to fix formatting only when the code itself changes. Especially when the problems are mostly just a few spaces around operators.
Re second commit: Having these types in GLSLElementTypes is a conscious decision - it is where all GLSLElementType instances are. RedefinedTokenElementType is a special case, but it is functionally closer to the fields in that class than to a generic GLSLElementType.
Re third: see above. Also, GLSLFlexLexer is a generated file, as mentioned on its first line, there is no point in editing that.
Re fourth: This is not very helpful. See here for a very nice overview of why.
Last two commits look interesting, please squash them and open a new PR for them.
A small tip: please don't open PRs with many unrelated changes (to any project) - it is hard to review them and even one rejected change will cause the whole PR to be rejected. Multiple smaller changes are always better (as long as the changes themselves are independent, of course).
Adrijaned
commented
5 years ago Re: Re: First commit - I took it mostly as a way to force myself to read through to codebase, and to really read and not just scroll quickly through each file. And since the code was already written, why not commit and PR just in case?
Re: Re: Second commit - alright, it just struct me as weird to have one File variable and one class declaration alongside of all these Element constants.
Re: Re: Third - whoops. It was just kinda annoying me and IDEA was complaining in the GLSLElement, and so I run project-wide refactoring alongside that because why not
Re: Re: Fourth - what I took from the linked article that's applicable here:
It might be possible though to create a new class, let's say GLSLPreprocessorParsing, where that function I extracted from could be moved in as whole, with the only entrypoint being parsePreprocessor()?
Re: Small tip: Yeah, I know, but since the code was not functioning completely correctly either way, I decided just for including it in this PR, so in case I am directly told to abandon it, I do not waste too much time on it. Will make a separate PR for that then.
Darkyenus
commented
5 years ago The gist of the linked article is that splitting code around for no good reason does not generally help. I agree that this is not 1:1 as the functions are inside a conditional, but they are still very related to each other and anybody reading/modifying one, should be aware of the others as well. I am a strong proponent of inlining - unless the code is massive, called from multiple places or a very clearly pure function with intuitive behavior, it should not have its own function (with some exceptions of course). In my experience, it is much easier to read a 1000 line sequential method than the same amount of code split across many tiny functions, which are only called once and may sometimes span several files.
If you open a PR for preprocessor block collapsing, mention that it is WIP and how is it failing and I'll check what the problem is.
So far: gone through all the files, edited whitespace and changed what wasn't to 1TBS, for consistency