search

robotframework / QuickStartGuide

Robot Framework Quick Start Guide
280 stars 309 forks source link

remove BDD prefix from keywords name, use OS dependent \n and fix typo #1

Closed laurentbristiel closed 9 years ago

laurentbristiel commented 9 years ago

For the BDD prefix, I find it better to not have them in the keyword name because 1) a given keyword could be used in "THEN" or "AND" section depending on the test 2) we want to show that the BDD prefix are in fact ignored by Robot and not part of the keyword name

pekkaklarck commented 9 years ago

  1. I typically also drop given/when/then prefixes when creating keywords, but I'm not sure is that a good idea here because that would need to be explained somehow. Possibly adding something like "notice that you can drop given/when/then/and prefixes when creating these keywords" into the existing comment in the keyword table would be enough, but I'm still not sure would all this be worth the effort.

    Related to this, I would find it more important to add some embedded arguments to these Gherkin-style keywords. These examples originate from time when embedded args weren't supported, but we obviously could change them now. I just didn't see any simple way to do it. Any ideas?

  2. \n should not be changed to ${\n} because it would make the test fail on Windows. Get File already converts \r\n to \n on Windows, so Should Contain must always use \n.
  3. The typo is a good catch.

I close this pull request because 1. above still needs work (and may not be worth the effort), 2. should not be done, and 3. can be easily fixed separately. This is also an example why a pr should generally contain only changes related to one topic.

pekkaklarck commented 9 years ago

After re-reading I actually like the current easiest better than easier. I'll leave it like that unless it is grammatically or otherwise wrong.

laurentbristiel commented 9 years ago

ok, thanks for the feedback. Fine with me. So the best way for such a PR would be to send 3 different PR?

pekkaklarck commented 9 years ago

Well, in general it's best to submit separate enhancements as separate PRs. The problem with submitting multiple enhancements together is that if they all cannot be accepted, for whatever reason, merging the others gets more complicated. No need to overdo that, though. A PR can contain one or two unrelated typo fixes, removal of whitespace, or similar, but you should be certain that everything can be accepted together.

What do you think about the BDD prefixes in the Quick Start Guide? I'm fine with dropping them but should add a note explaining the behavior briefly. Also, you have good ideas how to add embedded args to the examples?

laurentbristiel commented 9 years ago

OK, I understand for the PR. For the BDD prefix, I understand that dropping them would mean adding some more explanations which would make the whole flow less easy to read/understand, so let's leave it the way it is at the moment.

pekkaklarck commented 9 years ago

I actually did drop those prefixes in b43571a.