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

@timbltimbl 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

@RubenVerborghRubenVerborgh 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.

@RubenVerborghRubenVerborgh self-assigned this Jun 2, 2019
@RubenVerborghRubenVerborghforce-pushed the getFilePath branch 2 times, most recently from 4bffc6b to f4cba19CompareJune 2, 2019 20:08
@RubenVerborghRubenVerborgh changed the title Tweak put.js to avoid problem with URIs with spaces inRestrict knowledge of the URL/file mapping again to ResourceMapperJun 2, 2019
@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

@megothmegoth 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
@RubenVerborghRubenVerborgh merged commit 111e63b into masterJun 7, 2019
@RubenVerborghRubenVerborgh 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.

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

Reviewers

@kjetilkkjetilkkjetilk left review comments

+2 more reviewers

@RubenVerborghRubenVerborghRubenVerborgh left review comments

@megothmegothmegoth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@megothmegoth

Labels

bug

Projects

None yet

Milestone

No milestone

6 participants

@timbl@RubenVerborgh@kjetilk@michielbdejong@megoth