Skip to content

Restrict knowledge of the URL/file mapping again to ResourceMapper #1213

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.

Sign up for GitHub

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

Jump to bottom
Merged
RubenVerborgh merged 10 commits into from
Jun 7, 2019
Merged

Restrict knowledge of the URL/file mapping again to ResourceMapper #1213

RubenVerborgh merged 10 commits into from
Jun 7, 2019

Conversation

@timbl
Copy link
Contributor

@timbl timbl commented Jun 2, 2019 •
edited by RubenVerborgh
Loading

Closes #1211.
Closes #1212.

RubenVerborgh
RubenVerborgh previously requested changes Jun 2, 2019
View reviewed changes
Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Got some nits; will fix them myself.

@RubenVerborgh RubenVerborgh self-assigned this Jun 2, 2019
@RubenVerborgh RubenVerborgh force-pushed the getFilePath branch 2 times, most recently from 4bffc6b to f4cba19 Compare June 2, 2019 20:08
@RubenVerborgh RubenVerborgh changed the title Tweak put.js to avoid problem with URIs with spaces in Restrict knowledge of the URL/file mapping again to ResourceMapper Jun 2, 2019
@RubenVerborgh RubenVerborgh assigned megoth and unassigned RubenVerborgh Jun 2, 2019
@RubenVerborgh RubenVerborgh requested review from kjetilk and megoth June 2, 2019 21:07
@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jun 2, 2019

@megoth Too much knowledge about the URL/file mapping was leaking in other components, leading to bugs such as #1211, which #643 was created to prevent.

Unit tests run, but have not checked anything else. In particular, you want to look at:

  • allow.js
  • globbing (which now no longer is a hack, but has been reduced in functionality to only folder globs)
  • a skipped test which I think was incorrect

megoth
megoth approved these changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@megoth megoth left a comment

Choose a reason for hiding this comment

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

There is a lot happening here, but AFAICT it looks good ^_^

@kjetilk
Copy link
Member

kjetilk commented Jun 6, 2019

@RubenVerborgh perhaps you can hit merge on this when you're happy?

kjetilk
kjetilk reviewed Jun 6, 2019
View reviewed changes
@RubenVerborgh RubenVerborgh merged commit 111e63b into master Jun 7, 2019
@RubenVerborgh RubenVerborgh deleted the getFilePath branch June 7, 2019 08:22
@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jun 7, 2019

Thanks all!

@michielbdejong
Copy link
Member

michielbdejong commented Jun 17, 2019

@RubenVerborgh so did #1226 start happening when this PR was merged, or was that a coincidence?

@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jun 17, 2019

Yes; some of the knowledge that needed to be inside of ResourceMapper was encoded externally, and in a different way. Relying again on ResourceMapper brought to light the incorrect mapping.

@michielbdejong michielbdejong mentioned this pull request Jun 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kjetilk kjetilk kjetilk left review comments

+2 more reviewers

@RubenVerborgh RubenVerborgh RubenVerborgh left review comments

@megoth megoth megoth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@megoth megoth

Labels

bug

Projects

None yet

Milestone

No milestone

6 participants

@timbl @RubenVerborgh @kjetilk @michielbdejong @megoth