Skip to content

Simplify --splinter-headless - #123

Merged
tony merged 3 commits into
pytest-dev:masterfrom
tony:headless-argparse
Sep 12, 2020
Merged

Simplify --splinter-headless#123
tony merged 3 commits into
pytest-dev:masterfrom
tony:headless-argparse

Conversation

@tony

@tony tony commented May 13, 2019

Copy link
Copy Markdown
Member

It feels a bit strange doing true/false for a headless option.
It could be my own aesthetic / tastes.

argparse has 'store_true' for cases like this (see docs):

'store_true' and 'store_false' - These are special cases of 'store_const' used for storing the values True and False respectively. In addition, they create default values of False and True respectively..

So --headless sets it to True.

I can't think of a case where a user would want to accept False considering the default behavior is that, but if they did, they can always add their own option via pytest_addoption)

Not entering --splinter-headless infers False (this is backed by default values in pytest-splinter and probably user intuition). Entering --headless implies the user wants to run in headless mode.

@coveralls

coveralls commented May 13, 2019 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 87.681% when pulling ef8208d on tony:headless-argparse into a3406f1 on pytest-dev:master.

@bubenkoff
bubenkoff requested a review from youtux May 14, 2019 12:30
@youtux

youtux commented May 14, 2019

Copy link
Copy Markdown
Contributor

Sorry @bubenkoff, I don't have time to check this and the possible incompatibilities that may arise.

@tony

tony commented May 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@youtux @bubenkoff

I doubt there's a side effect.

Here's two examples of --headless being used as in this PR: https://github.com/seleniumbase/SeleniumBase/blob/v1.23.9/seleniumbase/plugins/selenium_plugin.py#L87, pytest-dev/pytest-selenium#135 (comment)

via SeleniumBase's README:

If you want to run tests headlessly, use --headless, which you'll need to do if your system lacks a GUI interface. Even if your system does have a GUI interface, it may still support headless browser automation.

As for this project in particular:

pytest-splinter defaults to False for headless (as a user would probably expect)

It's really unintuitive to pass true/false to an option when the default behavior is false.

There are corner cases (as always), but nothing is stopping them from writing their own option (e.g. --no-headless) via pytest_addoption or customizing the splinter_headless fixture in their conftest. We could also introduce --no-headless at a later time, but it's doubtful a request would arise.

@tony

tony commented May 31, 2019

Copy link
Copy Markdown
Member Author

@youtux @bubenkoff

This is behavior splinter should use by default. I think we should merge this.

There's no point in --splinter-headless false because splinter_headless option already defaults to false.

@tony
tony force-pushed the headless-argparse branch from e61c6eb to 78afa2a Compare July 20, 2019 22:37
@tony

tony commented Jul 20, 2019

Copy link
Copy Markdown
Member Author

Hi @youtux @bubenkoff, checking in, just rebased.

The reason I stand behind this is it's simpler.

Omission of the flag generally implies falsiness. Going by the https://en.wikipedia.org/wiki/Principle_of_least_astonishment.

There's no point in --splinter-headless accepting false if splinter_headless defaults to False.

At this point I already written my own --headless to get the equivalent behavior. It just is a bit odd to have a flag set to false when it defaults to false 😆

If it's not okay to put in it's okay to close this.

bubenkoff
bubenkoff requested changes Sep 11, 2020
View reviewed changes

@bubenkoff bubenkoff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tony please resolve conflicts

@tony

tony commented Sep 11, 2020

Copy link
Copy Markdown
Member Author

@bubenkoff Doing so now.

@tony
tony force-pushed the headless-argparse branch 2 times, most recently from 6eedc24 to c07d227 Compare September 11, 2020 21:56
@tony

tony commented Sep 11, 2020

Copy link
Copy Markdown
Member Author

@bubenkoff Done

bubenkoff
bubenkoff requested changes Sep 11, 2020
View reviewed changes

@bubenkoff bubenkoff left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please:

  • fix the documentation
  • add changelog record
  • add authors record
    also tests are broken with this

@tony
tony force-pushed the headless-argparse branch 2 times, most recently from 5e84e97 to 6d92c5b Compare September 12, 2020 17:56
tony added 3 commits September 12, 2020 13:10
@tony
Simplify --splinter-headless
a7ce0d1 
Not entering --splinter-headless infers False (this is backed by
default values in pytest-splinter and probably user intuition).
Entering --headless implies the user wants to run in headless mode.
@tony
Document --splinter-headless behavior
2236577 
Without argument, behavior is unchanged. `--splinter-headless` without
arguments defaults to 'true'. Optionally false or true can be passed.
@tony
Update authors and changelog
ef8208d
@tony
tony force-pushed the headless-argparse branch from 6d92c5b to ef8208d Compare September 12, 2020 18:10
@tony

tony commented Sep 12, 2020

Copy link
Copy Markdown
Member Author

@bubenkoff Better now?

@bubenkoff

bubenkoff commented Sep 12, 2020

Copy link
Copy Markdown
Member

please squash & merge

@tony
tony merged commit 51cd9bc into pytest-dev:master Sep 12, 2020
@tony
tony deleted the headless-argparse branch September 12, 2020 21:23
@mpasternak mpasternak mentioned this pull request Sep 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bubenkoff bubenkoff bubenkoff approved these changes
@youtux youtux Awaiting requested review from youtux

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tony @coveralls @youtux @bubenkoff