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

Version compare warning text #4842

Merged
merged 2 commits into from Nov 1, 2018
Merged

Conversation

@benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Oct 31, 2018

image

The old text mentions "library", however we want to reduce user confusion here, since "library" can have much different meanings in our project.

Since there are some projects on RTD that are documentation only and not necessarily connected to software releases, we suggest to go for just "documentation".

I also built the frontend, but I'm not really sure if that's common practice to do in the PR?

@codecov
Copy link

@codecov codecov bot commented Oct 31, 2018

Codecov Report

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

@@           Coverage Diff           @@
##           master    #4842   +/-   ##
=======================================
  Coverage   76.27%   76.27%           
=======================================
  Files         158      158           
  Lines        9981     9981           
  Branches     1261     1261           
=======================================
  Hits         7613     7613           
  Misses       2024     2024           
  Partials      344      344

Copy link
Member

@humitos humitos left a comment

Thanks for your contribution.

I agree with you that the message that RTD has currently is not generic enough and it's not widely useful for all the projects that we host. Also, this banner had a lot of different opinions from different users and we weren't able to make it in a generic way to make happy all of our project's owners.

As this change affect all the banners I'm sure that there will be people that want this specific message back in their project and we won't have a way to do this.

Because of these reasons, I created a Sphinx extension that you can install into your docs and customize the banner as you want: changing the message, the style, where it's shown, etc. I suggest you to try it and see if it does what you want.

I'm putting this into Design Decision so the core team can evaluate your changes. At first sight, I'm -1 on changing this message and bring this discussion and problems around the banner back. I would like to have those discussions in the extension outside Read the Docs core repository.

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Nov 1, 2018

Thanks for the feedback @humitos - I have studied #3481 -- however, I cannot find any particular opinions around the wording of the banner? I can see that you have contributed to analyzing various complex scenarios. But this PR doesn't try to play around with any of this.

To keep this constructive, I think it's better to not vote against small incremental changes that arguably improves an existing text. This is a documentation project, so I hope you get what I mean :)

It seems unlikely and highly speculative that someone has based their warning banner on the word "library" and I can't really see how the suggestion to write something more general would confuse users of a software library.

I'd like to avoid the extension solution for now because it will not have any backwards applicability (unless I dig around and rebuild old branches, overwrite tags etc). But thanks for writing it.

I think warning users about reading old doc versions is an issue that needs to be solved universally for RTD. Although it's a bit quirky, I'm happy with the current banner "injection" and just suggesting a simple text change.

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Nov 1, 2018

Supposing that it's a hot potato, it's great to wait for a wider approval of the text change.

@humitos
Copy link
Member

@humitos humitos commented Nov 1, 2018

I re-read your PR and I think I'm changing my mind.

You are not reading the most recent version of this documentation. X.Y.Z is the latest version available.

This sounds generic enough without mentioning library, software or anything tied to a particular "topic" but documentation which is what it is in the end.

So, forget what I said before and thanks for your reply :)

humitos
humitos approved these changes Nov 1, 2018
Copy link
Member

@humitos humitos left a comment

I changed my mind and I agree with this changes. Thanks again!

@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Nov 1, 2018

Thanks @humitos for the feedback and for being open minded despite the understandably long history of opinions on this matter :)

Should I keep the commit that rebuilt the frontend in this PR -- what is the standard for frontend-related PRs?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 1, 2018

Good change. The frontend assets will get regenerated before deploy, so I think it's fine as is. 👍

@ericholscher ericholscher merged commit c476aee into readthedocs:master Nov 1, 2018
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants