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

Make the STABLE and LATEST constants overridable #4099

Merged
merged 1 commit into from Sep 4, 2018

Conversation

@xrmx
Copy link
Contributor

@xrmx xrmx commented May 16, 2018

Moving them to settings and making life easier for downstreams
that want to use different labels.

Copy link
Member

@ericholscher ericholscher left a comment

This looks useful. I definitely want to maintain backwards compat though. If the goal is to simply make it able to be changed with a setting, a much less invasive change would be to have the constants be able to read from a setting, instead of changing every instance of it.

NON_REPOSITORY_VERSIONS = (
LATEST,
STABLE,
)
Copy link
Member

@ericholscher ericholscher May 29, 2018

This looks like a good change. There is downstream code that depends on this -- so we should maintain backwards compatibility here. If there is a simple way to introduce this change so that it simply reads from the settings and sets the constants value, it would be better.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented May 29, 2018

We are currently shipping the following commit:
italia@fc50de4

Downside of this is that we cannot ship the two together IIRC because of circular dependencies and tests still has them hardcoded so can't test out of the box with a different value in settings.

@agjohnson agjohnson added this to the Refactoring milestone Jun 8, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Also echoing the need to move this back to constants for downstream.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 8, 2018

Do you have any idea on how to make them configurable but still keep them in constants? I don't.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 18, 2018

Downside of this is that we cannot ship the two together IIRC because of circular dependencies and tests still has them hardcoded so can't test out of the box with a different value in settings.

@xrmx can you talk more about the reasons that italia@fc50de4 won't work?

I don't think we're ready to commit to this PR's approach in all our downstream dependencies, but I could perhaps be convinced if shown why the simpler settings-based approach can't work.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 18, 2018

@ericholscher it will work on its own and we are currently using it. But i think that using plain settings for something that should be configurable would be cleaner. Anyway if you are fine with that approach I can update the PR. The smaller delta we have from upstream the better.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 18, 2018

Yea, I think that is a much easier change to make at least for now. We can talk about the larger one later.

Makes the constants overridable from settings so that downstream
can use different ones.
@xrmx xrmx force-pushed the settingslatest branch from a3ae8de to a07663a Jun 19, 2018
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 19, 2018

Updated PR as agreed.

Copy link
Member

@ericholscher ericholscher left a comment

This looks like a good change. Curious @xrmx what are y'all using for these versions instead of latest and stable?

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 19, 2018

@ericholscher roughly the same but in italian :) draft instead of latest and stable is the same.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jul 4, 2018

@agjohnson care to update review after different proposed implementation please? Thanks!

humitos
humitos approved these changes Jul 5, 2018
Copy link
Member

@humitos humitos left a comment

Thanks!

The new approach it's better to me. I think we are safe to merge this PR.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jul 18, 2018

ping, we have a couple positive reviews, can we merge it please?

@humitos
Copy link
Member

@humitos humitos commented Aug 16, 2018

This is ready to merge. I'm assigning this to myself to merge after the migration that it's taking place this weekend.

@humitos humitos self-assigned this Aug 16, 2018
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Sep 3, 2018

@humitos any update?

humitos
humitos approved these changes Sep 4, 2018
Copy link
Member

@humitos humitos left a comment

Thanks!

@humitos humitos merged commit bde0088 into readthedocs:master Sep 4, 2018
1 check passed
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Sep 5, 2018

@humitos thanks!

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.

None yet

5 participants