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.

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

Switch from $type to type and children #34

Merged
merged 1 commit into from
Mar 10, 2017
Merged

Switch from $type to type and children #34

merged 1 commit into from
Mar 10, 2017

Conversation

dimaryaz
Copy link
Contributor

No description provided.

@dimaryaz dimaryaz self-assigned this Mar 10, 2017
kevinemoore
kevinemoore requested changes Mar 10, 2017
View reviewed changes
Copy link
Contributor

@kevinemoore kevinemoore left a comment

Choose a reason for hiding this comment

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

Looks good, but using constants instead of inline strings will help us avoid bugs.

quilt/tools/command.py
@@ -489,9 +489,9 @@ def _print_children(children, prefix, path):
def _print_node(node, prefix, child_prefix, name, path):
name_prefix = u"─ "
if isinstance(node, dict):
node_type = NodeType(node.pop(TYPE_KEY))
node_type = NodeType(node["type"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we still use constants for these? That way the linter will catch errors if we make typos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constants are kind of a workaround: the real solution is to use classes, and only use JSON for serialization/deserialization - but that's a bigger change.

@kevinemoore kevinemoore merged commit a70d4df into master Mar 10, 2017
@kevinemoore kevinemoore deleted the type branch March 10, 2017 18:49
akarve
akarve reviewed Mar 10, 2017
View reviewed changes
quilt/data.py
@@ -84,8 +83,7 @@ def _keys(self):
"""
group = self._store.get(self._prefix)
assert isinstance(group, dict), "{type} {grp}".format(type=type(group), grp=group)
del group[TYPE_KEY]
return group.keys()
return group["children"].keys()
Copy link
Member

Choose a reason for hiding this comment

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

prefer constant like CHILDREN_KEY

quilt/test/test_install.py
@@ -11,7 +11,7 @@
from six import assertRaisesRegex

from quilt.tools import command
from quilt.tools.const import HASH_TYPE, TYPE_KEY, NodeType
from quilt.tools.const import HASH_TYPE, NodeType
Copy link
Member

Choose a reason for hiding this comment

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

what dictates case for constants? (camel vs all-caps)

dimaryaz added a commit that referenced this pull request Nov 12, 2017
@dimaryaz
Merge pull request #34 from quiltdata/instance
5f2e441 
Rename blob to instance
nl0 pushed a commit that referenced this pull request Dec 7, 2019
@renovate @nl0
Update dependency vega to ^5.9.0 (#34)
0343abd
nl0 pushed a commit that referenced this pull request Dec 9, 2019
@renovate @nl0
Update dependency vega to ^5.9.0 (#34)
d187274
nl0 added a commit that referenced this pull request Dec 9, 2019
@nl0
Merge branch 'master' into akarve-master
8c33544 
* master:
  Update dependency vega to ^5.9.0 (#34)
  Update babel monorepo to ^7.7.5 (#31)
  Update dependency eslint-plugin-redux-saga to ^1.1.3 (#32)
  Update dependency terser-webpack-plugin to ^1.4.2 (#33)
  Update dependency @sentry/browser to ^5.10.1 (#27)
  Update dependency fetch-mock to ^8.0.1 (#28)
  Update dependency use-debounce to ^3.3.0 (#29)
  Update dependency react-app-polyfill to ^1.0.5 (#30)
  Update mui monorepo to ^4.7.1 (#25)
  Update dependency css-loader to ^3.2.1 (#21)
  Update dependency use-debounce to ^3.2.0 (#24)
  Update dependency eslint to ^6.7.2 (#22)
  Update dependency eslint-plugin-react to ^7.17.0 (#23)
  Update dependency aws-sdk to ^2.582.0 (#1)
  Update dependency style-loader to ^1.0.1
  Update dependency lint-staged to ^9.5.0
  Update dependency coveralls to ^3.0.9
  Update dependency connected-react-router to ^6.6.1
  Update renovate.json
nl0 added a commit that referenced this pull request Dec 11, 2019
@nl0
Merge branch 'master' into anon-credentials
1c5c784 
* master: (48 commits)
  Better package list ux pt. 1: larger hit area (whole tile) (#1439)
  Fix interrupted downloads causing corrupted manifests (#1438)
  Fixed missing @ for decorator (#1441)
  PyPi improvements - VERSION file and CircleCi support for alpha releases (#1427)
  Fix Telemetry double counting (#1426)
  preview bio files as plain text (#1435)
  Link to open data (#1434)
  Update dependency @material-ui/core to ^4.7.2 (#37)
  Update dependency @babel/preset-env to ^7.7.6 (#38)
  Update dependency eslint-import-resolver-webpack to ^0.12.0 (#39)
  Update dependency vega to ^5.9.0 (#34)
  Update babel monorepo to ^7.7.5 (#31)
  Update dependency eslint-plugin-redux-saga to ^1.1.3 (#32)
  Update dependency terser-webpack-plugin to ^1.4.2 (#33)
  Update dependency @sentry/browser to ^5.10.1 (#27)
  Update dependency fetch-mock to ^8.0.1 (#28)
  Update dependency use-debounce to ^3.3.0 (#29)
  Update dependency react-app-polyfill to ^1.0.5 (#30)
  Update mui monorepo to ^4.7.1 (#25)
  Update dependency css-loader to ^3.2.1 (#21)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akarve akarve akarve left review comments

@kevinemoore kevinemoore kevinemoore approved these changes

Assignees

@dimaryaz dimaryaz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dimaryaz @akarve @kevinemoore