iPlug2 / iPlug2

C++ Audio Plug-in Framework for desktop, mobile and web
https://iplug2.github.io
Other
1.89k stars 283 forks source link

Fixing whitespace violations #1008

Open tristan00b opened 11 months ago

tristan00b commented 11 months ago

Hi all,

I'm really excited to have found this project and would like to contribute by helping with the CMake config.

An issue I've been having so far is that files with whitespace violations mean that my editor is constantly trying to fix them upon saving. This makes for extra work when committing code, in order to separate whitespace changes from feature edits.

In addition to updating .editorconfig and .gitattributes to a) set a rule that there should be a single newline at the end of each file, and b) include EOL rules for CMake files, I'd like to propose performing a bit of cleanup.

The scope of the cleanup requires some discussion---which files to touch, and which to leave alone. I'm guessing everything in WDL can be left alone, for example. I also expect that someone will want to review the changes before merging, so consideration should also be paid to how commits are organized? E.g. depending on the number of files effected, one big commit might be too onerous, while a commit-per-file could also be annoying to deal with.

I'm happy to do the cleanup myself, but hoping to open up a discussion before moving ahead.

Cheers, Tristan


CHECK Is this an issue or just a question? If it's just a question, then post on the forum, or ask on slack, don't pollute our issue tracker.

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behaviour.

Expected behaviour A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

IMPORTANT DETAILS

Additional context Add any other context about the problem here.

olilarkin commented 11 months ago

Hi Tristan,

awesome that you want to get involved. I have also been wanting to start enforcing some code style on the codebase and have started this work with the clang-format branch here: https://github.com/iPlug2/iPlug2/tree/clang-format

Unfortunately there are several branches in flight at the moment with a lot of changes that I don't want to disrupt... the diffs will be massive if we reformat the master branch first. I am extremely busy at the moment and it's not looking like i will have a chance to address that any time soon.

I am happy to see your proposals on a branch and contribute to discussions, but I expect I can't merge any major formatting changes in the next few months

You are correct WDL and Dependencies should not be formatted at all

AlexHarker commented 11 months ago

I would agree that have repo local rules for dealing with line endings might reduce noise (for those who don't already have that as part of their global config - which we shouldn't assume). That would be a relatively easy change to incorporate ASAP. The clean-up is a different issue, but it would be worth seeing how many files it affects before we worry too much about that.

Thanks for taking this on @tristan00b!

olilarkin commented 11 months ago

we already have rules for line endings, but not newline at end of file.

olilarkin commented 11 months ago

anyhow... changes to e.g. .gitattributes that help get incoming PRs in the right format are great, but if some changes alter very many files simultaneously that might be a bit difficult to merge right now

AlexHarker commented 11 months ago

If I recall correctly changing .gitattributes doesn't enforce the change on old files - that's a separate step.

tristan00b commented 11 months ago

It sounds like dealing with formatting on a per PR basis is probably the best way to go then.

The rules I wanted to add to .gitattributes were for .cmake and .ps1 files, which aren't already there. If .cmake files use lf they'll be inline with the rule for .txt files, which covers CMakeLists.txt files. There is only one .ps1 file in repo that I've found, but perhaps there could be more in the future?

I can reopen my original PR which covers the changes to .editorconfig and .gitattributes. If that suffices for now, I'll move on to looking at the CMake config.

My approach so far has been to keep formatting changes isolated to separate commits, which should make reviewing easier. There honestly aren't that many issues that I've come across so far, it's just that their existence creates overhead when trying to make changes unrelated to formatting.

AlexHarker commented 11 months ago

Old should be able to confirm how many branches have cmake files - if they are just on the cmake branch then it would be fairly possible to incorporate changes there.

tristan00b commented 11 months ago

@AlexHarker Not exactly sure what you mean by 'old'.

olilarkin commented 11 months ago

I think he meant oli ;) nearly 40 so old works

On Sun, 8 Oct 2023 at 23:42, Tristan Bayfield @.***> wrote:

@AlexHarker https://github.com/AlexHarker Not exactly sure what you mean by 'old'.

— Reply to this email directly, view it on GitHub https://github.com/iPlug2/iPlug2/issues/1008#issuecomment-1752169366, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFACLTOTGSZHZXT4ZFRYYDX6MM5XAVCNFSM6AAAAAA5UTRBW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJSGE3DSMZWGY . You are receiving this because you commented.Message ID: @.***>

tristan00b commented 11 months ago

Ah. Damn, that make me old too then 😂

AlexHarker commented 11 months ago

Oh, to be still "nearly 40"....