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

Remove save_screenshot from Lint/Debugger #7853

Closed

Conversation

AndrewKvalheim
Copy link

The behavior of Lint/Debugger across possible debugging methods is inconsistent:

Method Interactive Reported
puts No No
save_page No No
save_screenshot No Yes
pry Yes Yes
save_and_open_page Yes Yes
save_and_open_screenshot Yes Yes

Capybara's save_screenshot probably shouldn't be reported, as it just writes a file noninteractively.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

`Capybara::Session#save_screenshot` isn't an entry point; it writes a
file noninteractively and is suitable for use in CI.

Overview:

| Method                     | Interactive | Reported |
| -------------------------- | ----------- | -------- |
| `save_page`                | No          | No       |
| `save_screenshot`          | No          | Yes → No |
| `save_and_open_page`       | Yes         | Yes      |
| `save_and_open_screenshot` | Yes         | Yes      |
AndrewKvalheim added a commit to AndrewKvalheim/osem that referenced this pull request Apr 5, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 15, 2020

Good points. I think we should probably also extend the cops documentation to make its purpose clearer. I also really dislike that we hardcoded the things we check for instead of providing them to the cop via configuration.

@ybiquitous
Copy link
Contributor

I strongly hope that this PR will be merged. 🙏 🙏 🙏

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 22, 2020

@ybiquitous It seems @AndrewKvalheim has lost interest in it, so I can suggest for you to pick it up if you're interested in driving it to the finish line.

@ybiquitous
Copy link
Contributor

@bbatsov Thanks, with pleasure! I'll open a new PR later. 💪

@ybiquitous
Copy link
Contributor

I've opened PR #8920!

@marcandre marcandre closed this Oct 22, 2020
hennevogel pushed a commit to AndrewKvalheim/osem that referenced this pull request Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants