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

Add an unresolver similar to our resolver #6944

Merged
merged 2 commits into from May 5, 2020
Merged

Add an unresolver similar to our resolver #6944

merged 2 commits into from May 5, 2020

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 22, 2020

Turn a URL into the component parts that our views would use to process them.

Look at the tests for an example.

@ericholscher ericholscher marked this pull request as draft April 22, 2020 00:18
readthedocs/core/unresolver.py Outdated Show resolved Hide resolved
readthedocs/core/unresolver.py Outdated Show resolved Hide resolved
@ericholscher ericholscher marked this pull request as ready for review May 2, 2020 16:34
@ericholscher ericholscher requested a review from a team May 2, 2020 16:34
@ericholscher ericholscher changed the title Add an example unresolver Add an unresolver similar to our resolver May 3, 2020
Turn a URL into the component parts that our views would use to process them.

This is useful for lots of places,
like where we want to figure out exactly what file a URL maps to
Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

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

This seems fine and reasonably well tested. I know this is primarily for the Embed API initially and that seems fine but if we need to ever unresolve multiple URLs, there's probably optimizations we could do because each call to this is going to involve multiple queries.


from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.proxito.middleware import map_host_to_project_slug
from readthedocs.proxito.views.utils import _get_project_data_from_request
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using it outside of proxito, it probably shouldn't be private (underscore prefixed).

humitos
humitos approved these changes May 5, 2020
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Having an object like this is great! It seems we are not using it yet in this code, but I think we can iterate over and refactor other places to start using it. Looks like a good win!

This unresolver, will help us to test deeply the URLConf defined by the user, as well 😄


# Handle returning a response
if hasattr(project_slug, 'status_code'):
return UnresolvedObject(None, None, None, None, None)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just return None instead?

Copy link
Member

Choose a reason for hiding this comment

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

It will be easier to perform checks by checking if parts than if parts.<somefield> --looks more intuitive to me, since <somefield> could be None and be a valid object as well (fragment, for example)

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense 👍


@override_settings(
PUBLIC_DOMAIN='readthedocs.io',
RTD_EXTERNAL_VERSION_DOMAIN='dev.readthedocs.build',
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think it's better if we use the real production value. It's easier to visualize and compare.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the dev value, which I think makes sense for testing.

PUBLIC_DOMAIN='readthedocs.io',
RTD_EXTERNAL_VERSION_DOMAIN='dev.readthedocs.build',
)
@pytest.mark.proxito
Copy link
Member

Choose a reason for hiding this comment

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

I don't think proxito settings are required for this. We are just testing an object.

Copy link
Member Author

Choose a reason for hiding this comment

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

They import the proxito code tho, at least currently

parts = unresolve('http://pip.readthedocs.io/en/latest/foo.html#fragment')
self.assertEqual(parts.project.slug, 'pip')
self.assertEqual(parts.language_slug, 'en')
self.assertEqual(parts.version_slug, 'latest')
self.assertEqual(parts.filename, 'foo.html')
self.assertEqual(parts.fragment, 'fragment')
Copy link
Member

Choose a reason for hiding this comment

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

I love these tests! Super easy to follow! 💯 More "unit test" that our current tests that needs to call Django internals

self.assertEqual(parts.filename, '')

def test_unresolver_single_version(self):
self.pip.single_version = True
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it required to .save the project here? 🤔

@ericholscher ericholscher merged commit 7d3efea into master May 5, 2020
1 of 2 checks passed
@ericholscher ericholscher deleted the unresolver branch May 5, 2020 13:20
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.

None yet

3 participants