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

Fix snippet reader tab and indent handling #161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jotschi
Copy link

@Jotschi Jotschi commented Mar 30, 2023

The PR addresses the following issues:

  • The snippet reader did not correctly apply the indentation for snippet sources which were indented using tabs. The minIndent calculation only checked spaces.

Snippet Source:

[tab][tab]line

Old Behaviour

[tab][tab]line

New Behaviour:

line
  • The snippet reader minIndent calculation was always 0 whenever the snippet contained empty newlines. Those empty lines will now correctly be handled.

Snippet Source:

[space][space]line1
[\n]
[space]line2

Old Behaviour

[space][space]line1
[\n]
[space]line2

New Behaviour:

line1
[\n]
[space]line2

@michael-o
Copy link
Member

I don't understand this change. It does not retain the original format, what does it address?

@Jotschi
Copy link
Author

Jotschi commented Oct 9, 2023

It fixes the indentation behaviour when the source contains tabs. Without the fix the indentation is not correctly handled. Without the fix the source code block has too many tabs and is not rendered in a nice format.

@michael-o
Copy link
Member

It fixes the indentation behaviour when the source contains tabs. Without the fix the indentation is not correctly handled. Without the fix the source code block has too many tabs and is not rendered in a nice format.

But looking at your examples it does remove tabs as well, no?

@Jotschi
Copy link
Author

Jotschi commented Oct 9, 2023

As far as I recall - no - tabs are not removed at all. See first example.

@michael-o
Copy link
Member

First snippet has two tabs, but expected does not have any. What do I miss?

@Jotschi
Copy link
Author

Jotschi commented Oct 9, 2023

The tabs from this example
[tab][tab]line
will not be removed. Thus the snippet looks ugly. The tabs should be removed.

@michael-o
Copy link
Member

The tabs from this example [tab][tab]line will not be removed. Thus the snippet looks ugly. The tabs should be removed.

Why? If the snippet is intended that way? How can we know what is right or wrong in snippet content and context?

@michael-o
Copy link
Member

@kwin, do you understand this PR?

@Jotschi
Copy link
Author

Jotschi commented Oct 9, 2023

The snippet renderer removes unnecessary indentation. It thus moves the code block left. A code example which is highly indented will thus not include the bogus indentation.

{
     {
snipped-start
             { 
                    // your code 
             }
snipped-end
      }
}

should result in

{ 
      // your code 
}

and not

             { 
                    // your code 
             }

@michael-o
Copy link
Member

But what if

             { 
                    // your code 
             }

is correct indentation?

@Jotschi
Copy link
Author

Jotschi commented Oct 9, 2023

The code clearly removes this indentation if it is done by using spaces. Using tabs it is not removed and thus looks broken.
I'm okay with it if that is the desired behaviour.

@michael-o
Copy link
Member

The code clearly removes this indentation if it is done by using spaces. Using tabs it is not removed and thus looks broken. I'm okay with it if that is the desired behaviour.

It is hard to say what is right, but I would say that a snippet should remain as-is and not modified, as ugly as it is.

@Jotschi
Copy link
Author

Jotschi commented Oct 9, 2023

I think the indentation is clearly removed in order to prevent horizontal scrollbars for included snippets.

@michael-o
Copy link
Member

I think the indentation is clearly removed in order to prevent horizontal scrollbars for included snippets.

I'd prefer to leave formatting as-is, but I am open to a snippet option which reverse-indents.

@michael-o
Copy link
Member

I'd like to perform a release this weekend. Anyone want to complete the discussion with me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants