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

Support for config-aware relative paths #1070

Closed
wants to merge 32 commits into from
Closed

Support for config-aware relative paths #1070

wants to merge 32 commits into from

Conversation

chriddyp
Copy link
Member

@chriddyp chriddyp commented Jan 7, 2020

@chriddyp
Copy link
Member Author

chriddyp commented Jan 7, 2020

Getting an odd testing error locally:

PYTHONPATH=~/dash/tests/assets pytest tests/unit/test_configs.py
Test session starts (platform: darwin, Python 3.6.2, pytest 5.2.2, pytest-sugar 0.9.2)
cachedir: .pytest_cache
rootdir: /Users/chriddyp/Repos/dash-stuff/dash, inifile: pytest.ini
plugins: mock-1.11.2, sugar-0.9.2, dash-1.7.0
collecting ... 
――――――――――――――――――――――――――――――――――――――――――――――――――――――― tests/unit/test_configs.py ――――――――――――――――――――――――――――――――――――――――――――――――――――――――
ImportError while importing test module '/Users/chriddyp/Repos/dash-stuff/dash/tests/unit/test_configs.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../dash-design-kit/vv/lib/python3.6/site-packages/_pytest/python.py:501: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
../dash-design-kit/vv/lib/python3.6/site-packages/py/_path/local.py:701: in pyimport
    __import__(modname)
../dash-design-kit/vv/lib/python3.6/site-packages/_pytest/assertion/rewrite.py:142: in exec_module
    exec(co, module.__dict__)
tests/unit/test_configs.py:15: in <module>
    from dash._utils import (
E   ImportError: cannot import name 'get_relative_path'

("/my-dash-app", "/page-1/sub-page-1", "/my-dash-app/page-1/sub-page-1"),

("/", "relative-page-1", "relative-page-1"),
("/my-dash-app", "/my-dash-apprelative-page-1", "/my-dash-apprelative-page-1"),
Copy link
Member Author

Choose a reason for hiding this comment

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

These relative URLs are ambiguous in this scenario as we don't know what page we're on.

For example, if we were on page /my-dash-app/this-page and then provided a relative-page-1, the resolved URL should be /my-dash-app/this-page/relative-page-1 but we can't provide that to the front end because we are unaware of this-page.

So, I thought we'd just keep it a simple prefix without any additional logic.

Relative URLs aren't documented in Dash nor used very much anyway.

@@ -1565,6 +1566,33 @@ def get_asset_url(self, path):

return asset

def get_relative_path(self, path):
Copy link
Member Author

Choose a reason for hiding this comment

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

Naming options:

  • get_relative_path
  • get_relative_url
  • get_rel_path
  • get_rel_url
  • relative_path
  • relative_url
  • rel_path
  • rel_url
  • relpath
  • relurl

Copy link
Member Author

Choose a reason for hiding this comment

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

  • It is technically a path and not a full URL although folks might think that "path" is corresponding to the file system path.
  • We use get_ in other places like get_asset_url. Do we keep it for consistency? I'd prefer to move get_asset_url usage over to this function, so folks just need to learn e.g. app.relpath('/assets/logo.png') and app.relpath('/page-2')

Ryan Patrick Kyle and others added 7 commits January 8, 2020 13:15
* ✨ async/dynamic support in R pkg deps

* 🔪 remove Authors block

* 🔪 insert package version number

* 🔨 use verbose 📦 title and desc from YAML

* ✨ autodetect vignettes

* check for author/maintainer address

* ✋ halt processing if fatal errors found

* autopopulate KeepSource

* auto-escape % in docstrings

* 🔪 filter examples from docstrings in R
@chriddyp
Copy link
Member Author

chriddyp commented Jan 8, 2020

ugh I rebased off of master instead of dev

@chriddyp chriddyp closed this Jan 8, 2020
@chriddyp chriddyp deleted the relative-paths branch January 8, 2020 20:07
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.

4 participants