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 hard code http scheme of short url #4656 #4886

Merged
merged 4 commits into from
Apr 27, 2018
Merged

remove hard code http scheme of short url #4656 #4886

merged 4 commits into from
Apr 27, 2018

Conversation

ripoul
Copy link
Contributor

@ripoul ripoul commented Apr 26, 2018

remove the hard code http. get the scheme of the request and use it to create the new url

superset/views/core.py Outdated Show resolved Hide resolved
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Wondering if we can use flask's url_for instead of building the url manually

@ripoul
Copy link
Contributor Author

ripoul commented Apr 26, 2018

something like this ? (I never use url_for)

url_for(directory,  r=obj.id)

@xrmx
Copy link
Contributor

xrmx commented Apr 26, 2018

@ripoul i think it would be more complicated than that to have the full url with scheme and host. Don't mind that, we can cleanup that later.

@ripoul
Copy link
Contributor Author

ripoul commented Apr 26, 2018

how can I make the test succeed ?

@xrmx
Copy link
Contributor

xrmx commented Apr 26, 2018

@ripoul you fix what hey report:
./superset/views/core.py:754:13: E122 continuation line missing indentation or outdented

that means that you have to readd the 4 spaces indentation you removed :)

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #4886 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4886   +/-   ##
=======================================
  Coverage   76.97%   76.97%           
=======================================
  Files          44       44           
  Lines        8537     8537           
=======================================
  Hits         6571     6571           
  Misses       1966     1966
Impacted Files Coverage Δ
superset/views/core.py 74.68% <ø> (ø) ⬆️

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 fa3da8c...b28090d. Read the comment docs.

@ripoul
Copy link
Contributor Author

ripoul commented Apr 26, 2018

@xrmx done ! ty for your help ! 😃

Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

Would you mind elaborating in the PR description as to why/when the hard coded “http” string is problematic? This would help provide reviewers with additional context.

superset/views/core.py Outdated Show resolved Hide resolved
@ripoul
Copy link
Contributor Author

ripoul commented Apr 26, 2018

for your first question : it's a probleme when your server accept only https request. In this case we would like to respond with the same scheme than the request to be sure the server can handle it.

@mistercrunch mistercrunch merged commit 510ae84 into apache:master Apr 27, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* remove hard code http scheme of short url apache#4656

* remove space

* add space

* remove temp var
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
* remove hard code http scheme of short url apache#4656

* remove space

* add space

* remove temp var
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* remove hard code http scheme of short url apache#4656

* remove space

* add space

* remove temp var
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants