Refactoring

[shangsuru]

Why

Hi! 👋 I want to use this Gem for a production system, and I would also like to contribute to this gem in the future.

What

In my first PR here, I did some refactoring and fixed code style issues. Thank you for reviewing 🙏🏻

Details

• Refactored Scimitar::Resources::Base.find_attribute(*path) into a one liner
• Removed parenthesis from methods that don't have parameters
• Removed redundant return statements
• Fixed typos in comments
• Removed 'version' from docker-compose file
• Simplified if statement conditions

There will be a similar PR for specs later

[shangsuru]

@bagp1 @pond Does someone have time to review this? 🙏🏻

[pond]

It's just down to me now, as @bagp has moved onto an exciting new role elsewhere. I did look at this a while ago but my time is heavily divided.

First off, before I get into this, I don't want it to seem too negative and I very much appreciate the time and effort you've put into the PR. Contributions even just of code style or typo fix-ups all help improve code quality and are welcomed. Just your comment typo fixes alone here are great. Thank you for all this.

Now, onto the bits I strugged with! I think much of what you've done is personal preference, but is counter to our own preference about coding style (such as parentheses on method calls, which we include often for clarity). Much of our use of such things comes from some 20 or so years now of experience with Rails and Ruby, and seeing what can cause confusion for coders especially less used to Ruby as a language. Wrapping a multi-line "if" condition in parens is another one. Removing return just makes the code fractionally harder to grok too (a method just bluntly ending in a dumb statement foo, rather than the proactive and precise return foo, can look like a mistake or otherwise just seem odd; it's micro-aggressions that increase cognitive burden, for sake of preferring a less cluttered aesthetic to the code - to which we are sympathetic, but argue, function over form).

Since Scimitar is a hybrid of other work, this coding style is of course not 100% consistent throughout and I realise that in itself might annoy some and can make it hard to judge what is expected! We don't have a coding style doc for the gem, after all.

Anyway, this means I'm on the fence. There are some things in here (such as comment typos) that I want to keep. Another one is where you removed the == true  on if skip_next_component == true and - while I often don't like truthy-falsy comparisons (type ambiguity and when is something truthy? puts "hello" if 0 is a good one) - in this case those are all bool flags and there are subsequent examples where they're checked without the "==", so it makes sense. You also removed parens that were in that sequence which were indeed pointless if (foo) ... => if foo ..., which again, makes sense. There's no ambiguity/cognitive burden difference arising for such a simple statement. Aesthetics therefore win.

So I guess...

• I'd prefer to keep the empty parens on method calls.
• It must to keep parens wrapping if conditionals on multi-line formatted ifs.
• I'd prefer to keep the explicit returns. In some cases, it must keep them; e.g. in def activerecord_columns(scim_attribute), where the removal means you have to read all the way down the flow to the end to know what the method's effective evaluation result is, and you don't see an explicit and early-exit return in there.

...and some more specific things...

• In app/models/scimitar/resource_type.rb you refactor to include an early exit condition. That's generally considered more dirty code. I personally don't mind those, but - at least in our internal code, and I see that by habit I've done that here and there in Scimitar too - appending an early exit warning of the form return foo # NOTE EARLY EXIT helps call the reader's attention to that possibility while also indicating that it's entirely intentional. I think I prefer your version, actually, but adding that # NOTE EARLY EXIT at the end of line 20 would be useful.
• The change to def self.find_attribute(*path) is a nice example of one-liner Ruby processing, but it's harder to understand. The code is much more dense and requires a much higher level of Ruby understanding. Unless we could prove significant performance improvement, I much prefer the multi-line predecessor as it is far easier to read and understand at a glance. I suspect the one-liner is actually a bit less efficient depending on exact data input, since it has no opportunity to break out of the iteration early, but instead processes the whole map, then separately runs a 'find' over the resulting array. But no arguments from me about it being a cute alternative! 😂

[shangsuru]

@pond Thank you for the reply and: Sorry, this is my first open source contribution on Github 🙏 I should have put the more opinionated code style adjustment in separate PRs to make it easier to review. Putting all changes in one PR makes effort to review much larger than the value the PR is giving.

• Reverted the change to remove no-args parenthesis and return statements
• Reintroduced parenthesis for multi-line ifs
• Added NOTE EARLY EXIT comment

Regarding the one-liner:

self.schemas.lazy.map { |schema| schema.find_attribute(*path) }.find(&:present?)

since it has no opportunity to break out of the iteration early, but instead processes the whole map

💡 FYI it doesn't process the whole map, because it uses lazy evaluation.
I also think the one-liner is quite cute... But I agree, it is only easier to read for people familiar with functional programming. So reverted it, too.

[ripa-developer]

Don't worry, it's a good PR, with a good description and clear intent, and I don't generally mind larger PRs at all.

Checks all passed and it looks good -> Merging! Thank you!

[ripa-developer]

Nice tidy-up and useful typo fixes. Thanks!