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

Add defaultWritable resource argument. #63

Merged
merged 1 commit into from
Nov 24, 2020

Conversation

lsowen
Copy link
Contributor

@lsowen lsowen commented Nov 12, 2020

If a filesystem doesn't support the access namespace for
getinfo(), allow the config to specify whether the returned
items should be treated as writeable or not.

@lsowen
Copy link
Contributor Author

lsowen commented Nov 12, 2020

This is meant to address #62

@lsowen
Copy link
Contributor Author

lsowen commented Nov 12, 2020

@lsowen lsowen mentioned this pull request Nov 16, 2020
@timkpaine
Copy link
Collaborator

im ok with this, @telamonian any thoughts?

Copy link
Contributor

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

I think it's a good idea. The only tradeoff is that it further clutters the constructor signature of FSManager, but that's not a huge deal at the moment.

@lsowen So far, this PR is looking good. It will need a few minor edits and additions:

  1. default_writable should be camelcase -> defaultWritable

  2. defaultWritable needs to get added to the metadata for each resource. Follow the same pattern as for resource['auth'] here:

if 'auth' not in resource:
resource['auth'] = 'ask'

  1. you'll also need to add defaultWritable to the "resource" schema definition here:

"resource": {
"description": "Specification for an fs resource",
"type": "object",
"additionalProperties": false,
"properties": {
"name": {
"description": "Display name of resource",
"type": "string"
},
"url": {
"description": "A url pointing to an fs resource, as per the PyFilesystem fsurl specification",
"type": "string"
},
"auth": {
"description": "Given any template {{VARS}} in the url, 'ask' (default) to open a dialog box asking for credentials, or `env` to pick up credentials from the server's environment variables",
"type": "string",
"enum": ["ask", "env", false],
"default": "ask"
}
}
}

This schema defines the user options for jupyter-fs that are settable via the Advanced Settings Editor in jlab.

@lsowen
Copy link
Contributor Author

lsowen commented Nov 16, 2020

hi @telamonian, one quick question...

The camel cased form (defaultWritable) should be used for the property field( https://github.com/jpmorganchase/jupyter-fs/pull/63/files#diff-d599eec819a1aa6275f09579454402bbf5af9a9147256bad279aec31929774b5R77), but I should continue to use snake case for the constructor parameter (https://github.com/jpmorganchase/jupyter-fs/pull/63/files#diff-fb15715ca2d4d4c0a2c7c444141c33783d2a64e0d6d9f46ca6249ac59c279ff1R115), correct?

@telamonian
Copy link
Contributor

hi @telamonian, one quick question...

The camel cased form (defaultWritable) should be used for the property field( https://github.com/jpmorganchase/jupyter-fs/pull/63/files#diff-d599eec819a1aa6275f09579454402bbf5af9a9147256bad279aec31929774b5R77), but I should continue to use snake case for the constructor parameter (https://github.com/jpmorganchase/jupyter-fs/pull/63/files#diff-fb15715ca2d4d4c0a2c7c444141c33783d2a64e0d6d9f46ca6249ac59c279ff1R115), correct?

hmm, that's annoying. It looks like snake_case is more prevalent across our python codebase than I'd realized (mostly but not entirely for reasons of compatibility with various Jupyter notebook apis).

@lsowen I went back and forth on this for a few minutes. Basically, I hate all of the options.

camelCase is the standard convention for jupyterlab option keys. Since that's the only user-facing change in this PR, I think it is important that we use defaultWritable in the the schema. On the other hand, camelCase would be awkward within the context of FSManager. So for now, I think the compromise you suggested is fine. I'll do a broader review of our naming practices in the near future

@lsowen
Copy link
Contributor Author

lsowen commented Nov 16, 2020

I'm guessing the snake case is probably due to the typical python naming convention (https://www.python.org/dev/peps/pep-0008/#function-and-variable-names).

I'll update the PR tomorrow AM based on your review comments.

Thanks!

@lsowen lsowen changed the title Add default_writeable resource argument. Add defaultWritable resource argument. Nov 17, 2020
Copy link
Contributor

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

A few minor formatting nits, but overall LGTM!

Once said nits are taken care of, I'll pull this in

js/schema/plugin.json Outdated Show resolved Hide resolved
jupyterfs/fsmanager.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
If a filesystem doesn't support the `access` namespace for
`getinfo()`, allow the config to specify whether the returned
items should be treated as writable or not.
Copy link
Contributor

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

LGTM!

@telamonian telamonian merged commit 7ec0f01 into jpmorganchase:master Nov 24, 2020
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.

4 participants