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

change sub path of the package url #36

Merged
merged 4 commits into from
May 9, 2018
Merged

Conversation

liyakun
Copy link
Contributor

@liyakun liyakun commented May 8, 2018

In the pull request 34, the modification to the returned package URL path is not enough, we also need to remove the sub path '/final/' from the returned URL.

@liyakun
Copy link
Contributor Author

liyakun commented May 8, 2018

@brew Sorry for the inconvenience, I did not realized that we also need to remove the '/final/' part from the returned package URL. I added another regex expression to remove it in this PR. Thanks.

Copy link
Contributor

@brew brew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I left a comment about a small code issue. Can you also run pylama conductor and fix the linting issues?

datapackage_url = \
re.sub(r'https?://datastore\.openspending\.org/',
'https://s3.amazonaws.com/datastore.openspending.org/',
datapackage_url)
re.sub(r'/final/datapackage.json', '/datapackage.json', datapackage_url))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should escape the period in r'/final/datapackage.json'.

@liyakun
Copy link
Contributor Author

liyakun commented May 9, 2018

@brew Thanks, I have made the period escape and solved linting issues.

@brew brew merged commit 6138f17 into openspending:master May 9, 2018
@brew
Copy link
Contributor

brew commented May 9, 2018

This should be deployed now.

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

Successfully merging this pull request may close these issues.

2 participants