Skip to content
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

Reduce indentations for nested loops #920

Merged
merged 3 commits into from
Jul 6, 2023
Merged

Reduce indentations for nested loops #920

merged 3 commits into from
Jul 6, 2023

Conversation

Rudomitori
Copy link
Contributor

closes #867

@belav
Copy link
Owner

belav commented Jun 25, 2023

A couple thoughts I had after looking at this change.

If the first foreach breaks, should the 2nd one still be indented? see this
image

Are we sure mixing while/for/foreach should not keep the indentation? When I see two foreach at the same level it makes sense what is happening. while followed by for is less obvious, but perhaps because I'm not used to it. see this
image

I do like what it does to code like this. I've traditionally been "always braces" but with an auto formatter you don't have the problem of indentation being incorrect. And nested foreachs without the extra indentation is a lot cleaner.
image

@shocklateboy92
Copy link
Collaborator

I definitely think this should only apply to nested loops of the same type.

If that makes things more complicated to implement (since we have to keep track of what the previous node was), I think we should apply this behavior to forceach only.

@Rudomitori
Copy link
Contributor Author

If the first foreach breaks, should the 2nd one still be indented?

I think, it will be better to keep more simple behavior and don't indent even if foreach breaks. It is more predictable and consistent.

Are we sure mixing while/for/foreach should not keep the indentation? When I see two foreach at the same level it makes sense what is happening. while followed by for is less obvious, but perhaps because I'm not used to it.

while and for reflect the same intention as foreach does and the intention is to iterate over something. foreach can be used only if we deal with classes that implement IEnumerable or has GetEnumerator and if they are not implemented we need to fall back to for or even while if there is no indexed access.

If we will keep the indentation for the case of the mixing loop types then we will force a developer to convert loops to one type.

  • if the type is foreach then the developer need to implement GetEnumerator.
    It is a good way to incapsulate the iteration logic, but the logic is not always the same for all usage of a class. Also IEnumarator is not a zero-cost abstruction and can be not applicable in performance sencitife code.
  • for requires an indexed access that can be hard to implement or meaningless at all.
  • while can be uncomfortable to use.

It will better to allow the developer to mix loop types in a way that is bast for his case and still has no unnecessary indentations.

But unfortunately, Rider keeps the indentation in this case and I didn't find out a option to change it. Also, Rider show a warning if there is no the indentation.

I think, we have to don't conflict with Rider behavior and keep the indentation in this case for now, but try to remove it in the future.

@shocklateboy92
Copy link
Collaborator

First of all, thank you very much for the detailed write-up - it's awesome when people are passionate about open source projects 👍

After taking your detailed points into consideration (making developers convert loop types, Rider rules, etc.), I think the best way forward for now is this:

  • Only reduce indentation for nested foreach loops without braces
  • Don't change the behavior based on whether there's a break involved

We'll see how that performs in the wild, what people's feedback is, etc.
At a later date, we can decide to reduce indentation for other loops as well.

That way,

  • Developers can easily opt-out of this behavior by putting braces (which is required per the style guide of a lot of projects anyway).
  • It's simple to implement
  • Doesn't cause anyone to convert loop types

I think it makes the most sense (even long term) because it covers the most common pattern: (iterating over nested iterators/collections). When someone sees a set of foreach statements on the same indentation level they can tell at a glance that someone is iterating over a nested collection. If the user chooses a different type of loop, that means they're doing something different and we should have the regular indented syntax to signal so.

@Rudomitori
Copy link
Contributor Author

I adjusted the code to reduce indentions only for the same loop type and did it for while, for and foreach loops. It was a very small change to do it not only for foreach

Copy link
Collaborator

@shocklateboy92 shocklateboy92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@shocklateboy92 shocklateboy92 enabled auto-merge (squash) July 6, 2023 19:48
@shocklateboy92 shocklateboy92 merged commit 65fbc9d into belav:main Jul 6, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested loops without brackets may be not indented
3 participants