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

Conversation

Projects
None yet
5 participants
@stsewd
Member

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

This comment has been minimized.

Member

stsewd commented Jan 11, 2018

Hope the tests don't panic!

@stsewd

This comment has been minimized.

Member

stsewd commented Jan 11, 2018

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

@humitos

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

This comment has been minimized.

@humitos

humitos Feb 15, 2018

Member

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).

This comment has been minimized.

@davidfischer

davidfischer Mar 5, 2018

Contributor

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

davidfischer commented Mar 5, 2018

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

@davidfischer

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

This comment has been minimized.

@davidfischer

davidfischer Mar 5, 2018

Contributor

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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);

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

This seems to render a different value, right?

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

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

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:

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

@stsewd stsewd removed the Status: blocked label Apr 18, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 18, 2018

Removing the blocking label since #3691 was closed

@ericholscher

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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 rtfd:master May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd: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