common icon indicating copy to clipboard operation
common copied to clipboard

Description retains original indentation.

Open mattwynne opened this issue 2 years ago • 10 comments

👓 What did you see?

In reviewing https://github.com/cucumber/cucumber-react/pull/97 it was observed that the description text retains the whitespace indentation from the original feature file.

✅ What did you expect to see?

Description text should have indentation whitespace removed.

mattwynne avatar Mar 22 '22 21:03 mattwynne

Unlike DocString, where the indentation has to be consistent or it's an error, there's no constraint on indentation in a description

Scenario: Whatever
      This
   Is
fine
   I
      believe
  Given foo
  When bar
  Then bam

It's not 100% clear what the rule would be for left-trimming the description lines - assume the least-indented one sets a baseline?

ciaranmcnulty avatar Mar 22 '22 22:03 ciaranmcnulty

IMO it should ideally error if the subsequent lines don't match the first one, and use that first line as the indent margin for the rest.

e.g.

Feature: Good indentation
    This
      is
    fine

=>

This
  is
fine

But

Feature: Bad indentation
    This
  is not OK

=>

Parse error at line 3: Indentation of description is inconsistent.

mattwynne avatar Mar 23 '22 05:03 mattwynne

IMO both of your representations are valid. I think one of the "bigger" issues at the moment is newlines are being absorbed.

So something like

As a foo
I want to bar
So I can baz

Should look "something like"

As a foo\nI want to bar\nSo I can baz

Currently it looks like

As a foo I want to bar So I can baz

luke-hill avatar Jun 23 '22 12:06 luke-hill

@mattwynne

IMO it should ideally error if the subsequent lines don't match the first one, and use that first line as the indent margin for the rest.

I think we talked IRL but here's an example that I think we'd want to support:

Feature: Password hashing

     * In 1999 we had a data breach that cost us $4bn
     * In 2018 we covered one up by the skin of our teeth 

    As a consequence we ensure we do not store passwords in plaintext

my preference would be a 'trim the smallest indent' also which would result in

 * In 1999 we had a data breach that cost us $4bn
 * In 2018 we covered one up by the skin of our teeth 

As a consequence we ensure we do not store passwords in plaintext

And I'd suggest it'd be a good idea to store indent: 2 on the relevant Message

ciaranmcnulty avatar Jun 23 '22 13:06 ciaranmcnulty

(I also want to be able to do this)

Feature: Da Boss

    ______                      _       ___         ___             
   / ___(_)__ ________ ____    (_)__   / _ \___ _  / _ )___  ___ ___
  / /__/ / _ `/ __/ _ `/ _ \  / (_-<  / // / _ `/ / _  / _ \(_-<(_-<
  \___/_/\_,_/_/  \_,_/_//_/ /_/___/ /____/\_,_/ /____/\___/___/___/
                                                         

ciaranmcnulty avatar Jun 23 '22 13:06 ciaranmcnulty

Note from meeting: Also consider how markdown styling affects sanitization (If it does).

EDIT: markdown requires 2 new lines to actually show a new line visually.

luke-hill avatar Jun 23 '22 14:06 luke-hill

So, regarding the indentation, and indentation only, at the community meeting, we suggest to retain the indentation using HEREDOC rule - we pick the less-indented line and remove that indentation from all the lines - and eventually keeping in the messages the indentation level that has been taken down

Any objection into doing that way?

If you have some objection, could you explain those and maybe suggest something else?

aurelien-reeves avatar Jun 23 '22 14:06 aurelien-reeves

IMO both of your representations are valid. I think one of the "bigger" issues at the moment is newlines are being absorbed.

So something like

As a foo
I want to bar
So I can baz

Should look "something like"

As a foo\nI want to bar\nSo I can baz

Currently it looks like

As a foo I want to bar So I can baz

@luke-hill I think this is a separate issue: it's purely to do with the choice that we've made in the HTML formatter to use Markdown to render the description field from the Gherkin. This is expected behaviour in Markdown, but I agree it can be surprising.

In order to keep this discussion focussed on the whitespace indentation, let's move that discussion about newlines over to https://github.com/cucumber/react-components/issues/225

mattwynne avatar Jun 23 '22 14:06 mattwynne

To throw out some other options, the Behat/Gherkin parser's rule is:

remove whitespace up to 2 chars more than the indent of the preceding keyword

Which I never noticed until today

That idea of 'looking at the keyword indent' may be a useful one?

ciaranmcnulty avatar Jul 09 '22 07:07 ciaranmcnulty

Although I am a great fusspot about lining things up in source code files, I think being opinionated about 2-space indentation in .feature files is a lost battle. .Net developers, for example, always insist on using tabs. Ugly as anything IMO, but it happens enough in real life that I think we could not start imposing any kind of standard over it.

mattwynne avatar Jul 12 '22 22:07 mattwynne