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

Output url_name instead of url_name_orig #36

Merged
merged 4 commits into from
Jul 16, 2015

Conversation

noisecapella
Copy link
Contributor

For LORE it's useful to keep the url_name attribute as url_name so that users can match old learning resources with the modified copy in LORE. My understanding is that url_name_orig is not used anywhere but I'd be happy to add a new option if this PR would break backwards compatibility.

cc @ichuang

Was getting:

    `TypeError: can't use a string pattern on a bytes-like object`

with non-trivial courses; in cases where the `if` statement above this
outdented section was not triggered.
@ShawnMilo
Copy link
Contributor

@noisecapella Please review #37; I had an error when manually testing this branch.

This fix isn't due to a bug introduced in your branch, but I think it's a good time to add it.

@noisecapella
Copy link
Contributor Author

Ok, it's merged

ShawnMilo added a commit that referenced this pull request Jul 16, 2015
Output url_name instead of url_name_orig
@ShawnMilo ShawnMilo merged commit 1983eb5 into master Jul 16, 2015
@ShawnMilo ShawnMilo deleted the feature/gs/preserve_url_name branch July 16, 2015 15:56
@pdpinch
Copy link
Member

pdpinch commented Jul 16, 2015

Uh, did we ever hear anything from Ike on this?

@ShawnMilo
Copy link
Contributor

Sorry, should this not have been merged? Should I revert it?

@ichuang
Copy link
Contributor

ichuang commented Jul 16, 2015

One of the most useful ways to employ xbundle is to make it rename Studio's
64-hex url_name values into something human readable, based on
display_name. That's why it's important to distinguish between url_name
and url_name_orig.

This change breaks this use case. I would prefer for it to be reverted.

-Ike.

On Thu, Jul 16, 2015 at 12:00 PM, Shawn Milochik notifications@github.com
wrote:

Sorry, should this not have been merged? Should I revert it?


Reply to this email directly or view it on GitHub
#36 (comment).

@ShawnMilo
Copy link
Contributor

Reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants