Improve displayed version name when building from PR #8237

abravalheri · 2021-06-06T13:38:55Z

Hello RTD maintainers and thank you very much for the tool, it is really amazing.

This PR is actually an improvement suggestion:

Currently, the displayed version in the case of builds originated from pull requests is simply a plain integer corresponding to the PR number in GitHub/GitLab. This can be a bit confusing, specially for new projects, because the PR numbers will be in the same range as possibly existing versions.

Therefore the idea is to make it more explicit when building from PRs by adding a PR- prefix to the PR number (or MR- in the case of GitLab).

A few notes:

You can see that there are some changes that were automatically introduced by pre-commit when I tried to run git commit. These format changes are not part of the original idea and not necessary to it. To facilitate distinguishing between the actual suggestion and the automatic changes I tried to split them into 2 separated commits (the second commit in this PR being the one corresponding to the changes created by pre-commit). There is one exception though regarding statements starting with if all([, which I had to manually tweak after the automatic code formatter, because the result was too weird and increased the number of errors/warnings returned by the linter.
When I run tox -e lint there are a few error messages corresponding to pre-existent code
When I run tox, there are a few failing tests that seem to be uncorrelated to the introduced changes. In fact, by checking other open PRs I can see that the same tests are failing elsewhere (for example here). They seem to be related to a change in API urls (or at least to the way they are being tested):
```
@@  @@
'urls': {
    'dashboard': {
-       'edit': 'https://readthedocs.org/dashboard/project/version/v1.0/edit/',
+       'edit': 'https://readthedocs.org/dashboard/project/version/v1.0/',
@@  @@
```
I originally thought about making the prefixes into something that cannot correspond to existing branch names and therefore very distinctive and not subject to name collisions, for example ~PR-#2 or ~MR-#2 (with the prefix being "~PR-#"/"~MR-#"; ~ is an invalid branch name character). However, since I don't know how the remaining of the RTD system would deal with these unconventional characters (maybe it is validated somewhere), I decided do simply leave it as "PR-"/"MR-". Please let me know if the original idea is safe and you would prefer it that way.

stsewd

Hi, thanks for the PR, we depend on the verbose_name being the identifier in a few places, changing this means we need to change a couple of other places that depend on this.

This can be a bit confusing, specially for new projects, because the PR numbers will be in the same range as possibly existing versions.

Do you have examples of projects that use just a number for versions? I think most projects use semver 1.x instead of just 1.

Where do you find showing the number confusing? I think we always put the number close to the text "Pull request #xxx".

docs/webhooks.rst

abravalheri · 2021-06-07T15:54:20Z

Thank you very much for the review @stsewd .

Could you point me out which other places the verbose_name is being used to identify the PR so I can also have a look and maybe improve? Of course if it is very complicated, please feel free to close this PR, in the end it is a matter of minor experience improvement. Unfortunately the whole setup for developing, specially when involving PRs was quite cumbersome for me, so I ended up just relying on the test suite.

The place I found it confusing was the version switch menu. The version shows up as a number (let's say 4), togheter with the other versions. So let's say you end up with something like v0, v0.1, v1, v1.1, v1.2, 4 in that menu.

After some thought we can figure it out due to the sequence continuity, but I think making it more explicit would be a nice improvement.

stsewd · 2021-06-07T16:06:24Z

The place I found it confusing was the version switch menu. The version shows up as a number (let's say 4), togheter with the other versions. So let's say you end up with something like v0, v0.1, v1, v1.1, v1.2, 4 in that menu.

I see, then I think we should implement that logic in our footer to make it more explicit that the version is the PR/MR identifier.

Maybe something like 1 (PR) or #1

abravalheri · 2021-06-07T16:39:24Z

That makes a lot of sense to me.

Is this string used in the footer the same as the one being set to READTHEDOCS_VERSION?
The reason why I tapped into the verbose_name is because I thought that was the one being used to set READTHEDOCS_VERSION and the footer.

Personally, I used the READTHEDOCS_VERSION to set html_title = f"{project} {READTHEDOCS_VERSION}" so would also be very happy if the env var is also more explicit.

Maybe if you point me in the right direction I could try to contribute with that? This was me first interaction with the code base, so not very sure on how to tackle this.

stsewd · 2021-06-07T16:50:40Z

Personally, I used the READTHEDOCS_VERSION to set html_title = f"{project} {READTHEDOCS_VERSION}" so would also be very happy if the env var is also more explicit.

Maybe if you point me in the right direction I could try to contribute with that? This was me first interaction with the code base, so not very sure on how to tackle this.

That env var is the slug of the version, not the name to be displayed, so we can't change that to include other text.

readthedocs.org/readthedocs/projects/tasks.py

Lines 326 to 334 in a91bd99

    
           def get_rtd_env_vars(self): 
        
               """Get bash environment variables specific to Read the Docs.""" 
        
               env = { 
        
                   'READTHEDOCS': 'True', 
        
                   'READTHEDOCS_VERSION': self.version.slug, 
        
                   'READTHEDOCS_PROJECT': self.project.slug, 
        
                   'READTHEDOCS_LANGUAGE': self.project.language, 
        
               } 
        
               return env

I think we can add a new env var like READTHEDOCS_VERSION_TYPE (here the type can be branch, tag, or external (that is for PRs), that way you could add a suffix/prefix when needed.

The footer can be changed here

readthedocs.org/readthedocs/api/v2/templates/restapi/footer.html

Lines 41 to 46 in a91bd99

    
           <dt>{% trans "Versions" %}</dt> 
        
           {% for version in versions %} 
        
           {% if version.verbose_name == current_version %} <strong> {% endif %} 
        
           <dd><a href="{{ version.get_subdomain_url }}{{ path|default_if_none:"" }}">{{ version.slug }}</a></dd> 
        
           {% if version.verbose_name == current_version %} </strong> {% endif %} 
        
           {% endfor %}

There you would need to check if the version is external

readthedocs.org/readthedocs/builds/models.py

Lines 900 to 906 in a91bd99

    
           @property 
        
           def is_external(self): 
        
               type = self.version_type 
        
               if self.version: 
        
                   type = self.version.type 
        
               return type == EXTERNAL

and add the prefix/suffix.

If you want to check your changes locally, you can check our docs at https://docs.readthedocs.io/en/stable/development/install.html

abravalheri · 2021-06-08T04:16:18Z

Thank you very much for the tips @stsewd.

I have rewritten the PR to reflect the discussion in the comments.

stsewd

Thanks for working on this!

readthedocs/api/v2/templates/restapi/footer.html

readthedocs/builds/models.py

readthedocs/api/v2/templates/restapi/footer.html

readthedocs/builds/utils.py

docs/builds.rst

readthedocs/projects/tasks.py

abravalheri · 2021-06-14T19:58:36Z

Thank you very much for the review and suggestions @stsewd, I submitted a new commit rebasing the new state of the master an adopting the changes you have described.

Regarding #4 (PR) x #4 (Pull Request), I agree that the second is even more explicit and therefore it would be better.

I suppose the layouts for the RTD theme and the floating menu for the other themes handle well long version names, right? For example this PR would have a name #8237 (Pull Request).
If that is fine, I can also change and remove the abbreviation. Maybe you want to discuss it with your colleagues? Please let me know and I will be happy to change that.

stsewd

Thanks! Looks good for me! /cc @readthedocs/core

ericholscher

This seems like a good change, but I'm a bit worried about the extension of the build context -- we are trying to move more away from using that data without it being a nicer UX. Why was that necessary for this PR?

readthedocs/builds/models.py

ericholscher · 2021-06-28T22:45:52Z

readthedocs/api/v2/templates/restapi/footer.html

-        <dd {% if version.verbose_name == current_version %} class="rtd-current-item" {% endif %}>
-          <a href="{{ version.get_subdomain_url }}{{ path|default_if_none:"" }}">{{ version.slug }}</a>
+        <dd {% if version == current_version %} class="rtd-current-item" {% endif %}>
+          <a href="{{ version.get_subdomain_url }}{{ path|default_if_none:"" }}">{{ version.explicit_name }}</a>


I don't think this is needed -- we will never show a PR build in this list. The slug is what will be shown in the URL, so I think we want the slug?

We were showhing the name for the current version, and the slug for the other versions, I think it makes sense showing the verbose name as this is how versions are originally named.

ericholscher · 2021-06-28T22:48:00Z

readthedocs/builds/models.py

+        """
+        external_origin = external_version_name(self)
+
+        if not external_origin:


We should check for if not self.type == EXTERNAL here or similar -- it's cleaner than depending on the side effects of another function.

abravalheri · 2021-06-30T12:31:50Z

Thank you very much for the review @ericholscher. I rebased the PR to master and added some changes to address some parts of your comments.

Regarding your comment:

This seems like a good change, but I'm a bit worried about the extension of the build context -- we are trying to move more away from using that data without it being a nicer UX. Why was that necessary for this PR?

I am not exactly sure if I understand it. What would it be the build context? Also not sure what it would mean to have "data without it being a nicer UX"... Sorry for that (it seems that you guys are used to some terms when talking about the project, but for me as an external contributor it is a bit difficult to understand).

docs/builds.rst

pmeier · 2021-09-18T13:57:55Z

Any update on this?

stale · 2022-03-02T10:34:23Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

humitos · 2022-03-07T10:38:57Z

@ericholscher can you take another look at this PR? It looks like a good addition but you have made some comments regarding the environment variable and build context. After a quick read of the comments, I think I'm fine adding the environment variable but not into the HTML context.

humitos · 2022-03-07T10:41:30Z

docs/webhooks.rst

+  and mark **Branch or tag creation**, **Branch or tag deletion** and **Pushes** events.
+  If you want to use Read the Docs to :doc:`preview documentation changes from pull
+  requests </pull-requests>`, also mark **Pull requests** events.


Suggested change

and mark **Branch or tag creation**, **Branch or tag deletion** and **Pushes** events.

If you want to use Read the Docs to :doc:`preview documentation changes from pull

requests </pull-requests>`, also mark **Pull requests** events.

and mark **Branch or tag creation**, **Branch or tag deletion**, **Pushes** and **Pull requests** events.

I think it's better to just mark those without too many explanations. Otherwise, people will need to modify the webhook if they want that feature later.

ericholscher · 2022-03-09T21:25:40Z

@humitos

After a quick read of the comments, I think I'm fine adding the environment variable but not into the HTML context.

Agreed. I think ENV vars are fine, but in some ways we're just changing around where the data is coming from. I don't want to keep adding things there without a bit of thought either on namespacing at least. I'm 👍 on exposing this data though if there isn't a good way to get it currently.

humitos

I re-review this PR and I think it's good to be merged. I left just small improvements that should be easy to adjust.

@abravalheri do you have the time to update this PR so we can merge it?

docs/builds.rst

humitos · 2022-03-10T10:05:35Z

docs/builds.rst

+   - If ``READTHEDOCS_VERSION_TYPE`` is ``external``,
+     it means that the version was built from pull request. See :doc:`/pull-requests`.


I'd say to move this text to the description of the variable itself instead of in a note. After that, we remove the bullets and keep just the initial paragraph of the note which is great.

The version switch menu in RTD currently uses `version.slug` to display the name of the versions. For external versions coming from PRs that corresponds to a simple integer number. For example, let's consider a project have 3 existing "regular versions": v0.1, v1 and v2. In the 3rd PR submitted to GitHub, a version with slug '3' would be created, which would result in the version switch menu containing something like: v0.1, v1, v2, 3. This can be slightly confusing, since it is trick to differentiate between regular branches/tags used for versions and PR numbers, specially when the project is starting. This commit adds `explicit_external_name` and `explicit_name` to the version model and uses them in the footer template to try to make a more clear differentiation between regular builds and builds triggered by PRs.

These variables make it easy for configuration scripts to differentiate between regular builds and builds triggered by PRs. Moreover, they are quite handy to setting up `html_title` in a way that matches the version being displayed in the RTD's version switch menu.

abravalheri · 2022-03-10T13:04:32Z

Hi @humitos, thank you very much for the latest reviews.
I tried to rebase, fix the conflicts and apply the latest changes you suggested. I hope it is good.

I can see that the ci/circleci: checks are failing and in their logs they mention pre-commit.
When I try to run the same command locally, pre-commit changes a bunch of stuff that was not touched by this PR... (some parts were touched, but the coding style adopted was in keep with the current code-style found in the files).

I can add another commit adding the automatic changes suggested by pre-commit, but I am afraid it will introduce a lot of unrelated changes that will cloud the diff in the PR...

humitos · 2022-03-10T13:33:28Z

I can add another commit adding the automatic changes suggested by pre-commit, but I am afraid it will introduce a lot of unrelated changes that will cloud the diff in the PR...

Yeah, this is unfortunate because we are in the process of "blackify" the whole codebase starting only with the files touched by each PR. I just checked running tox -e pre-commit by myself in your branch and I think it will be fine adding a commit with those changes.

The PR is already reviewed and I'd say it's ready to be merged. In any case, we can skip that particular commit if we want to re-re-re-view it again 😄 --but I don't think it will be needed.

I'm sorry for all the time and work this took to you and thanks for it.

abravalheri · 2022-03-10T13:41:37Z

think it will be fine adding a commit with those changes.

Perfect, I just submitted 2eab243 with the changes automatically generated by pre-commit.

I got the configuration file from https://github.com/readthedocs/common/blob/main/pre-commit-config.yaml

Hopefully that will be enough to fix the problems in the CI and not cause any unrelated error 😅 🤞.

Pass `READTHEDOCS_VERSION_TYPE` and `READTHEDOCS_VERSION_NAME` that were missed because a merge conflict. These variables were added in #8237

jsquyres · 2022-03-29T18:56:21Z

It looks like at least part of this functionality was (inadvertently?) reverted when #9002 was merged.

Was that intentional? I'll open up a separate issue for this, but wanted to mention this on the PR where this functionality was introduced.

ericholscher · 2022-03-30T21:45:41Z

This should now be fixed, please let me know if it isn't.

stsewd reviewed Jun 7, 2021

View reviewed changes

docs/webhooks.rst Outdated Show resolved Hide resolved

yayapichfficial approved these changes Jun 7, 2021

View reviewed changes

abravalheri force-pushed the improve-pr-version-name branch from f228147 to afc75f7 Compare June 8, 2021 04:12

abravalheri force-pushed the improve-pr-version-name branch from afc75f7 to 4f7d853 Compare June 14, 2021 15:23

stsewd reviewed Jun 14, 2021

View reviewed changes

abravalheri added a commit to abravalheri/readthedocs.org that referenced this pull request Jun 14, 2021

Addopt review suggestions from readthedocs#8237

a9ecaa7

abravalheri force-pushed the improve-pr-version-name branch from 4f7d853 to a9ecaa7 Compare June 14, 2021 19:53

stsewd approved these changes Jun 28, 2021

View reviewed changes

stsewd requested a review from a team June 28, 2021 18:59

ericholscher reviewed Jun 28, 2021

View reviewed changes

abravalheri added a commit to abravalheri/readthedocs.org that referenced this pull request Jun 30, 2021

Addopt review suggestions from readthedocs#8237

65ddf48

abravalheri force-pushed the improve-pr-version-name branch from a9ecaa7 to 967776a Compare June 30, 2021 12:19

abravalheri added a commit to abravalheri/readthedocs.org that referenced this pull request Jun 30, 2021

Address suggestions from code review in readthedocs#8237

967776a

ericholscher reviewed Jun 30, 2021

View reviewed changes

docs/builds.rst Outdated Show resolved Hide resolved

stsewd mentioned this pull request Sep 7, 2021

Expose another environment variable for PR builds #8461

Closed

stale bot added the Status: stale Issue will be considered inactive soon label Mar 2, 2022

stale bot removed the Status: stale Issue will be considered inactive soon label Mar 7, 2022

humitos reviewed Mar 7, 2022

View reviewed changes

humitos reviewed Mar 10, 2022

View reviewed changes

abravalheri added 6 commits March 10, 2022 11:20

Addopt review suggestions from readthedocs#8237

8153f7c

Address suggestions from code review in readthedocs#8237

9594435

Fix error introduced by automatic confict resolution

af7ce40

Address the latest comments in the code review

483182c

abravalheri force-pushed the improve-pr-version-name branch from 967776a to 483182c Compare March 10, 2022 12:40

abravalheri requested review from a team as code owners March 10, 2022 12:40

Adopt automatic changes from pre-commit in CI

2eab243

ericholscher merged commit 81c8834 into readthedocs:master Mar 14, 2022

abravalheri deleted the improve-pr-version-name branch March 15, 2022 10:31

humitos added a commit that referenced this pull request Mar 16, 2022

Environment variables

17f0ca2

Pass `READTHEDOCS_VERSION_TYPE` and `READTHEDOCS_VERSION_NAME` that were missed because a merge conflict. These variables were added in #8237

jsquyres mentioned this pull request Mar 29, 2022

READTHEDOCS_VERSION_TYPE env variable support removed #9051

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve displayed version name when building from PR #8237

Improve displayed version name when building from PR #8237

abravalheri commented Jun 6, 2021

stsewd left a comment

abravalheri commented Jun 7, 2021

stsewd commented Jun 7, 2021

abravalheri commented Jun 7, 2021

stsewd commented Jun 7, 2021

abravalheri commented Jun 8, 2021

stsewd left a comment

abravalheri commented Jun 14, 2021

stsewd left a comment

ericholscher left a comment

ericholscher Jun 28, 2021

stsewd Jun 29, 2021

ericholscher Jun 28, 2021

abravalheri commented Jun 30, 2021

pmeier commented Sep 18, 2021

stale bot commented Mar 2, 2022

humitos commented Mar 7, 2022

humitos Mar 7, 2022

ericholscher commented Mar 9, 2022

humitos left a comment

humitos Mar 10, 2022

abravalheri commented Mar 10, 2022

humitos commented Mar 10, 2022

abravalheri commented Mar 10, 2022

jsquyres commented Mar 29, 2022

ericholscher commented Mar 30, 2022

		- If ``READTHEDOCS_VERSION_TYPE`` is ``external``,
		it means that the version was built from pull request. See :doc:`/pull-requests`.

Improve displayed version name when building from PR #8237

Improve displayed version name when building from PR #8237

Conversation

abravalheri commented Jun 6, 2021

stsewd left a comment

Choose a reason for hiding this comment

abravalheri commented Jun 7, 2021

stsewd commented Jun 7, 2021

abravalheri commented Jun 7, 2021

stsewd commented Jun 7, 2021

abravalheri commented Jun 8, 2021

stsewd left a comment

Choose a reason for hiding this comment

abravalheri commented Jun 14, 2021

stsewd left a comment

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher Jun 28, 2021

Choose a reason for hiding this comment

stsewd Jun 29, 2021

Choose a reason for hiding this comment

ericholscher Jun 28, 2021

Choose a reason for hiding this comment

abravalheri commented Jun 30, 2021

pmeier commented Sep 18, 2021

stale bot commented Mar 2, 2022

humitos commented Mar 7, 2022

humitos Mar 7, 2022

Choose a reason for hiding this comment

ericholscher commented Mar 9, 2022

humitos left a comment

Choose a reason for hiding this comment

humitos Mar 10, 2022

Choose a reason for hiding this comment

abravalheri commented Mar 10, 2022

humitos commented Mar 10, 2022

abravalheri commented Mar 10, 2022

jsquyres commented Mar 29, 2022

ericholscher commented Mar 30, 2022