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

Ignore masked references to FileAttachment. #129

Merged
merged 1 commit into from Aug 31, 2020
Merged

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Aug 2, 2020

We currently consider all calls to FileAttachment to be file references, even if the FileAttachment symbol is masked in the current scope. I feel this is overzealous and inconsistent: if we don’t consider the reference to FileAttachment to be an external reference (i.e., a cell input), then we shouldn’t consider the call to be a file reference, either.

This does mean that you can circumvent the static enforcement of file references:

(FileAttachment => FileAttachment("alphabet" + ".csv"))(FileAttachment)

But that’s already possible today:

(f => f("alphabet" + ".csv"))(FileAttachment)

Deferring review request until Monday. 🤙

@j-f1
Copy link

j-f1 commented Aug 2, 2020

This does mean that you can circumvent the static enforcement of file references:

Would it make sense to forbid the use of the FileAttachment identifier anywhere outside of a CallExpression’s callee then?

@mbostock
Copy link
Member Author

mbostock commented Aug 2, 2020

Would it make sense to forbid the use of the FileAttachment identifier anywhere outside of a CallExpression’s callee then?

I don’t see that being necessary (or particularly helpful). The goal isn’t to strictly prevent dynamic invocation, but to have predictable, consistent semantics about what a “well-formed” file reference looks like.

@mbostock mbostock requested a review from jashkenas August 3, 2020 16:15
@mbostock mbostock merged commit 07e7787 into master Aug 31, 2020
@mbostock mbostock deleted the masked-file-attachment branch August 31, 2020 18:47
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.

None yet

3 participants