Addons: resolve URLs for file tree diff API response #11749
Conversation
This works for now, but we also want to sort them alphabetically as well.
readthedocs/proxito/views/hosting.py
Outdated
| "current": resolver.resolve( | ||
| project=project, | ||
| filename=filename, | ||
| version_slug=version.slug, | ||
| ), | ||
| "source": resolver.resolve( | ||
| project=project, | ||
| filename=filename, | ||
| version_slug=latest_version.slug, | ||
| ), |
There was a problem hiding this comment.
A couple of notes:
- We are using version_a and version_b as naming (similar to diff), not current/source.
- Maybe just have two fields (version_a_base_url and version_b_base_url) at the top level response, and the client can use that information to build the final URL (similar to what we do in search). This will also make it easier to link to the top level pages of both versions.
- Even if we prefer to have this information per file, you can just resolve both base urls once in the backend, and then concatenate to each filename (resolver.resolve_version will give you this information, and that method should be used when you already have access to the version object to avoid hitting the DB again).
There was a problem hiding this comment.
We are using version_a and version_b as naming (similar to diff), not current/source.
I would like to use a name with more semantic meaning that communicates what's the source version and what's the current version. version_a and version_b does not communicate that in a clear way --I find them confusing.
I'm happy to change source/current name for something different that conveys the semantic meaning I'm looking for if we have something better here, tho. cc @ericholscher
Maybe just have two fields (version_a_base_url and version_b_base_url) at the top level response, and the client can use that information to build the final URL.
I want to avoid manipulating URLs in the frontend as much as possible. The backend resolver is the source of truth here and I expect we will be making changes to it in the future. So, I want to stick to it.
resolver.resolve_versionwill give you this information
I used this method and it reduced 5 queries in total 👍🏼 . We are at 30 DB queries when FTD is enabled, which is pretty similar to when it's not, so we are good here. We can concentrate more in the performance later if we notice it's needed, tho.
There was a problem hiding this comment.
I thought about this for a bit, and came up with some other options (current and original) which imply changes a bit more explicitly. I'm not 100% sure the best approach here. I agree a/b is an odd thing to surface for folks who aren't familiar with diff, but I agree we want it to be consistent, and if there's existing ways to discuss this it's not a bad idea to reuse them. I know git diff's expose a/b as a concept, so it's probably fine to reuse existing patterns here, instead of reinventing our own?
There was a problem hiding this comment.
OK, I don't like version_a and version_b, but it seems there is consensus to move forward with that. So, I will move forward with those names.
readthedocs/proxito/views/hosting.py
Outdated
| "current": resolver.resolve( | ||
| project=project, | ||
| filename=filename, | ||
| version_slug=version.slug, | ||
| ), | ||
| "source": resolver.resolve( | ||
| project=project, | ||
| filename=filename, | ||
| version_slug=latest_version.slug, | ||
| ), |
There was a problem hiding this comment.
I thought about this for a bit, and came up with some other options (current and original) which imply changes a bit more explicitly. I'm not 100% sure the best approach here. I agree a/b is an odd thing to surface for folks who aren't familiar with diff, but I agree we want it to be consistent, and if there's existing ways to discuss this it's not a bad idea to reuse them. I know git diff's expose a/b as a concept, so it's probably fine to reuse existing patterns here, instead of reinventing our own?
Added a new addon to show all the filenames and URLs that were added, deleted or modified in the current PR compared to the LATEST version.  I'm rendering this data in a new notification decoupled from the existing one on purpose. I don't want to entangle the code of these two different addons for now, in particular while we are under development/testing. I also see that in the future they may have different UI elements, or the data will be integrated in a more bigger UI element (eg. toolbar) or similar. We can discuss more how this addon will grow and where is a good place to add this data and make that work in a following PR. For now, I want something that we can start testing internally in our own projects. ### Examples    Closes #409 Requires readthedocs/readthedocs.org#11749
Instead of using `version_a` and `version_b`, we are using `current` and `base` for these field names. These names are standard GitHub/Git names, for example when creating pull requests and you have to select the "base branch". * Refs: #11762 (comment) * Refs: #11749 (comment)
Instead of using `version_a` and `version_b`, we are using `current` and `base` for these field names. These names are standard GitHub/Git names, for example when creating pull requests and you have to select the "base branch". * Refs: #11762 (comment) * Refs: #11749 (comment)
Resolve all the filename into URLs, so the frontend can use these links without manipulating them. We don't want re-implement the resolver in the frontend.
Now, we return the filename, the URL for the current version, and the URL for the source version of the filename changed.
Closes readthedocs/addons#409