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

Refactor object model to support friendlier REPL interactions #177

Merged
merged 14 commits into from
Feb 7, 2017

Conversation

yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented Feb 1, 2017

This branch changes how we internally represent and interact with statements in the aleph code. Most of the existing code operated on the StatementMsg type, which is the unmarshaled protobuf as a JS object. This led to lots of tedious and error-prone cases around the oneof fields, and made for cryptic and unfriendly output for queries at the REPL, etc.

So this branch adds a Statement class, which is convertible to/from the protobuf form, and which has some handy helper methods that were formerly free functions (e.g. statementSource is now .source()). Each type of statement body gets its own subclass, and you can use instanceof checks to figure out which is which if you need to, although hopefully most of what you want will be available on the main Statement object which will paper over the details.

A bonus of having classes is that it's easier to define a custom .inspect() method, which lets us override the default printed representation at the REPL and in console.logs.

I also changed the .remoteQuery method to unwrap the query results from the protobuf's they're delivered in, which, combined with the custom .inspect methods, makes for a much nicer REPL experience.

namespace list query (old):

א > remote.query('SELECT namespace FROM *')
[ { value: { simple: [Object] } },
  { value: { simple: [Object] } },
  { value: { simple: [Object] } },
  { value: { simple: [Object] } } ]

namespace list query (new):

א > remote.query('SELECT namespace FROM *')
[ 'images.500px',
  'images.dpla',
  'images.pexels',
  'mediachain.schemas' ]

Combined with the custom .inspect methods, it makes fetching a statement (and its data) much nicer. The remoteQueryWithData method returns a statement with a special "expanded" body, and has a nice clean representation at the console:

א > remote.queryWithData('SELECT * FROM images.500px LIMIT 1')
[ { id: '4XTTM4K8sqTb7xYviJJcRDJ5W6TpQxMoJ7GtBstTALgh5wzGm:1478269180:3738110',
    publisher: '4XTTM4K8sqTb7xYviJJcRDJ5W6TpQxMoJ7GtBstTALgh5wzGm',
    namespace: 'images.500px',
    timestamp: 1478269180,
    signature: 'wvJZqV86g2f1C+OvUITuvuVrFjMaF/lsXgoxo9Jr8nGqsPzJMNob3/h4+gEVxkTLwHe6wmFpjAT1TNvKEhkMDw==',
    body:
     { object:
        { schema: { '/': 'QmYGRQYmWC3BAtTAi88mFb7GVeFsUKGM4nm25SBUB9vfc9' },
          data:
           { artist_name: 'Oliver Andrews',
             aspect_ratio: 1.5058823529411764,
             date_created: '2016-05-19T14:37:26-04:00',
             dedupe_hsh: '95f7e73fde2c0360',
             description: 'Flower buds forming on flowering dogwood (the red stemmed variegated variety).',
             keywords:
              [ 'flowers',
                'nature',
                'flower',
                'bud',
                'plants',
                'plant',
                'black and white',
                'canon',
                'garden',
                'flora',
                'outdoors',
                'buds',
                'shrub',
                'dogwood',
                'cornus',
                'flowering dogwood',
                'ukspring',
                'canon70d',
                'canonuk' ],
             license_tags: [ 'CC BY-NC-ND' ],
             license_url: null,
             licenses: [ { name: 'CC BY-NC-ND', name_long: 'CC BY-NC-ND' } ],
             native_id: '500px_154574729',
             nsfw: false,
             origin: 'https://500px.com/photo/154574729/dogwood-buds-by-oliver-andrews',
             sizes: [ { content_type: 'image/jpeg', height: 682, width: 1024 } ],
             source:
              { name: '500px',
                url: 'https://500px.com/photo/154574729/dogwood-buds-by-oliver-andrews' },
             source_dataset: '500px',
             source_tags: [ '500px.com' ],
             title: 'Dogwood Buds' } },
       refs: [ '500px_154574729' ],
       deps: [ 'QmYGRQYmWC3BAtTAi88mFb7GVeFsUKGM4nm25SBUB9vfc9' ],
       tags: [] } } ]

Anyway, there's still some cleanup and testing to do here; a few places still work with the "raw" protobuf messages instead of the new classes, and I haven't tested all the new code yet.

TODO:

  • tests
  • finish replacing code that uses StatementMsg protobufs directly
  • possibly replace getters (e.g. .objectIds) on statements with plain methods, since flow doesn't like getters
  • make sure that the "expanded statement body" is a good idea... I'm not sure if the current impl is where it's at

@parkan
Copy link
Contributor

parkan commented Feb 2, 2017

Much, much better! 👍

@parkan parkan mentioned this pull request Feb 2, 2017
@parkan
Copy link
Contributor

parkan commented Feb 3, 2017

Seeing some incompatible array/buffer calls, but no accessor complaints in my flow

@yusefnapora
Copy link
Contributor Author

I added a line to the .flowconfig to enable "unsafe" getters/setters. Flow doesn't trust them by default since they can be side effecting.

@yusefnapora
Copy link
Contributor Author

btw, my note above about whether the expanded statement bodies were a good idea was mostly because I was considering whether to make a more general "resolve object" interface, which could be backed by e.g. a Datastore instance (or even a stream to a remote node).

The problem there is that you'd need the interface to be async, which is more awkward to use at the repl, etc. I'm pretty fine with the expanded bodies as they are now, I think.

@yusefnapora yusefnapora changed the title WIP - Refactor object model to support friendlier REPL interactions Refactor object model to support friendlier REPL interactions Feb 6, 2017
Copy link
Contributor

@parkan parkan left a comment

Choose a reason for hiding this comment

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

Damn, that's a lot of refactoring! Did you use the jetbrains refactoring features?

*/
function setEquals<T> (a: Set<T>, b: Set<T>): boolean {
if (a.size !== b.size) return false
for (const elem of a.values()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

huh I guess you can use const instead of let because it's scoped to inside, neat

return this.publisher
}

return this.body.statements[0].source
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about the semantics of this, don't think we have a good way to frame it so I guess this is fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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


constructor (contents: {object: string, refs?: Array<string>, deps?: Array<string>, tags?: Array<string>}) {
super()
this.objectRef = contents.object
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess object isn't actually a reserve word but it sure feels like it

expandObjects (source: Map<string, Object>): StatementBody {
const object = source.get(this.objectRef)
if (object == null) {
// FIXME: should we just return the un-expanded body instead?
Copy link
Contributor

Choose a reason for hiding this comment

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

depends on our treatment of "naked"/"uninflated" objects/statements, I'm inclined to say "yes" but right now we should expect to always have the objects in sane conditions

@yusefnapora
Copy link
Contributor Author

heh, yeah I've been using intellij since they added flow support; it makes crap like this so much easier.

@parkan parkan merged commit b5a2d10 into master Feb 7, 2017
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

2 participants