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

Fix the --browser option #504

Merged
merged 2 commits into from Aug 27, 2021
Merged

Fix the --browser option #504

merged 2 commits into from Aug 27, 2021

Conversation

Stannislav
Copy link
Contributor

Description

The problem was that webbrowser.open expects a URI and doesn't
work with relative local paths.

Solution: use pathlib to convert reporter.output_path to a URI
using pathlib.Path(reporter.output_path).resolve().as_uri().

Motivation behind this PR?

See #503

What type of change is this?

Bug fix.

Checklist

  • I have added a changelog entry / my PR does not need a new changelog entry. Instructions.
  • I have added/updated unit tests. Instructions.
  • New and existing unit tests pass locally with my changes. Instructions
  • I have self-documented code my changes by adding docstring(s) and comment(s). Instructions
  • Current PR does not significantly decrease the code coverage and docstring coverage.
  • My code follows the style guidelines of this project.
  • I have run ScanAPI locally and manually tested my changes. Instructions.
  • I have squashed my commits. Instructions.

Issue

Closes #503

The problem was that webbrowser.open expects a URI and doesn't
work with relative local paths.

Solution: use pathlib to convert reporter.output_path to a URI
using pathlib.Path(reporter.output_path).resolve().as_uri().
@Stannislav Stannislav requested review from a team as code owners August 26, 2021 21:09
@github-actions
Copy link

@Stannislav your pull request is missing a changelog!

@camilamaia
Copy link
Member

@Stannislav it works for me on macOS ⭐️ thanks for the fix!

Just one detail, would you mind adding a changelog entry, please?

@Stannislav your pull request is missing a changelog!

@Pradhvan
Copy link
Member

@camilamaia great! I could not test this since I am on Linux. I did leave a text on the contribution channel on discord for anyone who can test this out.

Copy link
Member

@Pradhvan Pradhvan left a comment

Choose a reason for hiding this comment

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

LGTM ⭐

@Stannislav the PR just needs a changelog entry. Rest is good to go. 🚀

@codecov-commenter
Copy link

Codecov Report

Merging #504 (7827ef4) into main (f304230) will not change coverage.
The diff coverage is 75.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #504   +/-   ##
=======================================
  Coverage   96.94%   96.94%           
=======================================
  Files          22       22           
  Lines         720      720           
=======================================
  Hits          698      698           
  Misses         22       22           
Impacted Files Coverage Δ
scanapi/reporter.py 88.88% <75.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f304230...7827ef4. Read the comment docs.

Copy link
Member

@camilamaia camilamaia left a comment

Choose a reason for hiding this comment

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

🌟

@Stannislav
Copy link
Contributor Author

@camilamaia @Pradhvan changelog updated in 7827ef4

For some reason I thought the bug was introduced in the same version :)

@Pradhvan Pradhvan merged commit 0a5734b into main Aug 27, 2021
@Pradhvan Pradhvan deleted the issues/503 branch August 27, 2021 14:50
camilamaia pushed a commit that referenced this pull request Apr 8, 2022
* Fix the --browser option

The problem was that webbrowser.open expects a URI and doesn't
work with relative local paths.

Solution: use pathlib to convert reporter.output_path to a URI
using pathlib.Path(reporter.output_path).resolve().as_uri().

* Add a changelog entry
@astenstrasser astenstrasser mentioned this pull request Apr 12, 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

Successfully merging this pull request may close these issues.

--browse option does not work on MacOS
4 participants