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

Description retains original indentation. #26

Open
mattwynne opened this issue Mar 22, 2022 · 10 comments
Open

Description retains original indentation. #26

mattwynne opened this issue Mar 22, 2022 · 10 comments

Comments

@mattwynne
Copy link
Member

mattwynne commented Mar 22, 2022

πŸ‘“ What did you see?

In reviewing cucumber/react-components#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.

@ciaranmcnulty
Copy link
Contributor

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?

@mattwynne
Copy link
Member Author

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.

@luke-hill
Copy link
Contributor

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

@ciaranmcnulty
Copy link
Contributor

ciaranmcnulty commented Jun 23, 2022

@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
Copy link
Contributor

(I also want to be able to do this)

Feature: Da Boss

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

@luke-hill
Copy link
Contributor

luke-hill commented Jun 23, 2022

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.

@aurelien-reeves
Copy link
Contributor

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?

@mattwynne
Copy link
Member Author

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 cucumber/react-components#225

@ciaranmcnulty
Copy link
Contributor

ciaranmcnulty commented Jul 9, 2022

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?

@mattwynne
Copy link
Member Author

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.

@mpkorstanje mpkorstanje transferred this issue from cucumber/common Nov 8, 2022
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

No branches or pull requests

4 participants