react-juce icon indicating copy to clipboard operation
react-juce copied to clipboard

Overhauled the formatting and designs.

Open jrlanglois opened this issue 4 years ago • 7 comments

There are many changes here

  • Stylistically changed many explicit variable types to use auto.
  • Sprinkled const-ref here and there, where necessary.
  • Flipped the wacky Yoda conditions.
  • Added a .gitattributes to make things consistent for cross-platform development.
  • Fixed up how third-party warnings are managed by adding a .h and .cpp to start/finish or push/pop ignoring warnings.
  • Lots of macro changes to use what JUCE offers, instead of the hand-rolled shenanigans.
  • Updated the .gitignore so the repo can cope with vscode and MSVC stuff.
  • Fixed Duktape's C++11 detection for MSVC.
  • Made Doxygen comments follow JUCE's conventions, and also reformatted them here and there.
  • Fixed up a lot of the C++11 (and up) std::function stuff to make it more readable.
  • Added more exporters to the unit test project.
  • Added a JUCE module compatible macro to be able to enable/disable unit tests from the Projucer and its Cmake equivalents.
  • Tried to avoid implicit bool and pointer usage (you'll see explicit != nullptr and == nullptr comparisons now).
  • Swapped typedef usage for using.
  • Removed all inclusions within the module and moved them into just the master module files.

Still in progress

  • Migrating implicit casts of juce::var to using static_cast<Type> so as to avoid any compiler faffery.
  • Need to fix some errors in the unit tests; errors that have cropped up after my slew of changes.

Why are you doing this?! ARE YOU MAD?!

Yes... Probably?

So, my approach here was to organically run through and do various passes to dig up any and all of the things that are inconsistent, potentially broken, missing, and whatever else qualifier. Some of the code is just plain crusty...

And no worries - I get that some contributors are just having fun, and the overall goal is to make something useful. Also, I'm aware that not everybody knows JUCE super well. Really I'm just pitching in to look at all of this from a framework development POV.

And yeah, this PR can be broken down into more digestable chunks! Treat this as a proof of concept and to gather ideas for what can come next.

jrlanglois avatar Dec 08 '20 20:12 jrlanglois

@nick-thompson Have a gander...

jrlanglois avatar Dec 08 '20 20:12 jrlanglois

@jrlanglois this looks great. A Herculean effort!

One key point, It's a pretty enormous diff! This is quite likely to cause merge conflicts upon rebasing etc as I suspect some other things will get merged first, any chance of breaking it up a little? i.e. the build/module changes as one PR. Stylistic changes as another.

Couple other points.

In regards to all the empty doxygen additions here. (@nick-thompson will correct me if I'm wrong on this one). I think the idea is that users will very rarely need to read into C++/Native other than View and ReactApplicationRoot. The vast majority of documentation will focus on the JS level using the existing docsify system. Nick and I were discussing this a while back and we'd probably aim to keep the vast majority of documentation in one place. So we'd probably document those two classes in docsify pages and largely avoid doxygen.

In regards to the various formatting changes, an ideal thing here would be a clang-format config. I think there are some out in the wild on the JUCE forum which follow the JUCE style guide. Obviously this is separate from the overall refactors preferring auto etc which I think look great. Appreciate it'd be rather painful to separate those refactors from formatting though ....

@nick-thompson what do you think?

JoshMarler avatar Dec 09 '20 15:12 JoshMarler

In regards to the various formatting changes, an ideal thing here would be a clang-format config. I think there are some out in the wild on the JUCE forum which follow the JUCE style guide. Obviously this is separate from the overall refactors preferring auto etc which I think look great. Appreciate it'd be rather painful to separate those refactors from formatting though ....

Yeah I think clang-format and prettier config in repo, along with some checks in CI for compliance and probably some easily-initialised pre-commit hooks will be super valuable. No reason to waste anybody's time worrying about formatting down the line when a lot of that burden can be tooled away. Seems you've already decided to do this anyway https://github.com/nick-thompson/blueprint/issues/24

olidacombe avatar Dec 11 '20 12:12 olidacombe

Thanks for the feedback folks! I'm glad to see there's interest. As I mentioned in my original post - treat this T H I C C PR as merely conceptual. Though looking back on it, it's fairly straightforward to break all of this down into sizeable chunks.


@JoshMarler

In regards to all the empty doxygen additions here. (@nick-thompson will correct me if I'm wrong on this one). I think the idea is that users will very rarely need to read into C++/Native other than View and ReactApplicationRoot.

I'd like to make the case that JUCE users will be interested in this, what with it being a JUCE module... This means C++ will likely be the first thing people think of, particularly when it comes to rolling out their own C++ classes for the JS mechanisms to interact with. Documentation will be good to have in place and will make it more digestible.

This might be less useful in granular terms (eg: ShadowView), but for the classes that users are most likely to touch it seems worthwhile.

Also, keeping in mind the freshness and early stages of this module, docs might help debug when things go awry.

jrlanglois avatar Dec 11 '20 14:12 jrlanglois

Thanks for the feedback folks! I'm glad to see there's interest. As I mentioned in my original post - treat this T H I C C PR as merely conceptual. Though looking back on it, it's fairly straightforward to break all of this down into sizeable chunks.

@JoshMarler

In regards to all the empty doxygen additions here. (@nick-thompson will correct me if I'm wrong on this one). I think the idea is that users will very rarely need to read into C++/Native other than View and ReactApplicationRoot.

I'd like to make the case that JUCE users will be interested in this, what with it being a JUCE module... This means C++ will likely be the first thing people think of, particularly when it comes to rolling out their own C++ classes for the JS mechanisms to interact with. Documentation will be good to have in place and will make it more digestible.

This might be less useful in granular terms (eg: ShadowView), but for the classes that users are most likely to touch it seems worthwhile.

Also, keeping in mind the freshness and early stages of this module, docs might help debug when things go awry.

Totally fair comment. It would be nice if there was a clean single solution to generate docs for both sides of the API if possible. @nick-thompson, any thoughts on this?

JoshMarler avatar Dec 11 '20 14:12 JoshMarler

Wow! @jrlanglois herculean effort indeed, very very much appreciated. And cheers to each of you for the discussion here.

High level comments: yea clang-format + prettier + automated clang-tidy/whatever CI checks is definitely the sustainable approach here, and one that we've planned to do but not gotten around to. Documentation, yea, @JoshMarler said it well; we're definitely aligning behind the idea of keeping the docs in one place, and that the most of the documentation will (and should) be geared towards the js side, so I'm starting to think it would be more maintenace than value to also support doxygen for the C++ side. That said, adding those docblock comments will indeed be helpful for contributors looking to understand the project, so I'm in favor of at least keeping up with our comments in the code!

@jrlanglois I see the breakout PRs, I'll go through and address each individually in more detail. Shall we consider this one closed then?

nick-thompson avatar Dec 12 '20 21:12 nick-thompson

Ah, misread my notifications, I'm seeing now that those were issues opened, not breakout PRs! Ignore the end of my previous comment then :)

I'll read through the issues and leave comments in detail where I have them though. And yes, to @JoshMarler's other point, I think breaking this down into bite sized PRs will be the way forward!

nick-thompson avatar Dec 12 '20 21:12 nick-thompson