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

Consistent version format #3504

Merged
merged 3 commits into from May 24, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jan 11, 2018

This closes #2357

A time ago travis failed, so this #3402 was proposed. Now while looking at #2357 I think we only need to return a single value (a version only have a single unique slug).

The js, didn't break because of some weird js implicit cast.

var currentURL = window.location.pathname.replace('rtfd', ['hi'])
console.log(currentURL)
// This works!

But as mentioned on the spec, must be a string not a list https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace.

Note: Thanks @lethosor for his initial work on this lethosor@7d84130

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jan 11, 2018

Hope the tests don't panic!

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jan 11, 2018

The tests failed, should update the tests or put this how was initially and change the js instead?

Copy link
Member

@humitos humitos left a comment

The problem I see making this as it should be is that we don't know who relis on this code (since it's accessed by javascript). So, I don't personally feel comfortable on changing it because I can't be sure that everything will continue working.

So, I'd say that we need to fix our Python tests for this, make some manual QA on different pages to see how the JS is affected and check that everything continue working and finally ask @agjohnson or @ericholscher if this won't blow up something else :)

@@ -43,7 +43,7 @@ def get_version_compare_data(project, base_version=None):
}
if highest_version_obj:
ret_val['url'] = highest_version_obj.get_absolute_url()
ret_val['slug'] = (highest_version_obj.slug,)
ret_val['slug'] = highest_version_obj.slug
Copy link
Member

@humitos humitos Feb 15, 2018

This should be a single item. We were already biten by this issue and I saw a couple of drawbacks when changing this, and finally it stayed as it was (like a tuple).

Copy link
Contributor

@davidfischer davidfischer Mar 5, 2018

I agree that this should be a single item.

Technically, this changes a public API endpoint response which would involve a major rev of the API (ugh, one of the few downsides of semver). If instead we classify it as a bugfix, then perhaps it's ok.

Do we have any idea why it was an array instead of a simple string? This appears to date back to at least #1499 when the v2 footer API was added. Do we think it was a bug the whole time? There was additional discussion in #3394 (comment).

@stsewd stsewd force-pushed the consistent-version-format branch from c51a325 to a6c89d1 Feb 16, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 16, 2018

@humitos I update just one test file and everything is green now :). I think the only part this was used was on the banner and footer.

@humitos
Copy link
Member

@humitos humitos commented Feb 20, 2018

@davidfischer can you take a look at this PR? The Python code seems to be correct, but since there is a Javascript object that is different now in the response I want to be sure that this won't break other possible usages of this.

Not sure how to know this, though. So, maybe you know better :)

@RichardLitt RichardLitt requested a review from davidfischer Feb 21, 2018
@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Mar 5, 2018

Sorry I missed this. I'm looking at it now.

Copy link
Contributor

@davidfischer davidfischer left a comment

From our functionality, this appears to be ok. However, I don't think I have the insight to know who is relying on this API endpoint.

I hate passing the buck, but I think @agjohnson or @ericholscher have to chime in.

@@ -43,7 +43,7 @@ def get_version_compare_data(project, base_version=None):
}
if highest_version_obj:
ret_val['url'] = highest_version_obj.get_absolute_url()
ret_val['slug'] = (highest_version_obj.slug,)
ret_val['slug'] = highest_version_obj.slug
Copy link
Contributor

@davidfischer davidfischer Mar 5, 2018

I agree that this should be a single item.

Technically, this changes a public API endpoint response which would involve a major rev of the API (ugh, one of the few downsides of semver). If instead we classify it as a bugfix, then perhaps it's ok.

Do we have any idea why it was an array instead of a simple string? This appears to date back to at least #1499 when the v2 footer API was added. Do we think it was a bug the whole time? There was additional discussion in #3394 (comment).

@RichardLitt RichardLitt requested a review from ericholscher Mar 5, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 6, 2018

The footer API is an internal API, so it should be OK to change how it works, as long as the client code changes too.

The main tricky thing with JS is that sometimes we are shipping the JS with each build, so we have to support older versions with embedded JS. This client code comes from the server though, so I think this change should be 👍.

The best way to test this is to build a local docset with master, then another docset with this branch. Then see if both work with the branch's JS.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 6, 2018

I believe this is blocked on our inability to build our JS artifacts though, unless that has been resolved. We need to ship the Python & JS code at the same time.

@@ -23,7 +23,7 @@ function init(data) {
warning
.find('a')
.attr('href', currentURL)
.text(data.version);
.text(data.slug);
Copy link
Member

@humitos humitos Mar 23, 2018

This seems to render a different value, right?

What's is version and what's slug here?

Copy link
Member

@humitos humitos Mar 23, 2018

Is version used in another places? We should check that we can still access it now it's not a tuple anymore.

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

What's is version and what's slug here?

I think your past self alreay answer that question 😄 #2357 (comment). Also #2357 (comment)

I did a grep and it is used here:

https://github.com/rtfd/readthedocs.org/blob/c5d102a4b119e191fd6a7ea201c776e84dd177f8/readthedocs/core/static-src/core/js/doc-embed/search.js#L16

But again thanks to the implicit cast of js everything was good (at the end everyone was expecting a string here p:)

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

Removing the blocking label since #3691 was closed

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 24, 2018

This has bothered me for ages. Will your work around the version comparison & Sphinx extension depend on this @humitos? I think it looks ready to merge from my perspective.

@humitos
Copy link
Member

@humitos humitos commented May 24, 2018

@ericholscher no, I don't think this will affect my work.

I'm merging it since you all agree with this changes and looks solid to me also.

Thank you all!

@humitos humitos merged commit 15972f1 into readthedocs:master May 24, 2018
1 check passed
@stsewd stsewd deleted the consistent-version-format branch May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants