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

Implement valuable value pointer #59

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Implement valuable value pointer #59

wants to merge 14 commits into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jun 30, 2021

Example

use valuable::*;

#[derive(Valuable)]
struct Struct1 {
    x: String,
    y: Struct2,
}

#[derive(Valuable)]
struct Struct2 {
    z: String,
}

struct Visitor;

impl Visit for Visitor {
    fn visit_value(&mut self, value: Value<'_>) {
        println!("{:?}", value);
    }
}

fn main() {
    let value = Struct1 {
        x: "a".to_owned(),
        y: Struct2 {
            z: "b".to_owned(),
        },
    };

    let mut visitor = Visitor;

    visit_pointer!(value.x, visitor);   // "a"
    visit_pointer!(value.y, visitor);   // Struct2 { z: "b" }
    visit_pointer!(value.y.z, visitor); // "b"
}

TODO

  • function to create Pointer from str
  • decide APIs
    • naming
      • types and methods in pointer module
      • rename valuable-derive to valuable-macro(s)? (because visit_pointer! macro is proc-macro)
  • write docs
  • add tests

@Keats
Copy link

Keats commented Jul 4, 2021

Can there be a Pointer::from_str? When querying data at runtime on arbitrary paths, you can't build a macro.

@taiki-e
Copy link
Member Author

taiki-e commented Jul 6, 2021

@Keats Good point. It definitely makes sense to provide the ability to dynamically create a pointer from str.

@Keats
Copy link

Keats commented Jul 7, 2021

@taiki-e ping me when it's ready it works for a str, I'll be happy to try it out!

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Looks great to me. I added an inline thought. If this works for @Keats though, I say we move forward.


fn visit_named_fields(&mut self, named_values: &NamedValues<'_>) {
if let Segment::Field(name) = self.pointer.path[0] {
if let Some(value) = named_values.get_by_name(name) {
Copy link
Member

Choose a reason for hiding this comment

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

The challenge will be to avoid using get_by_name here in favor of using a field as that would be faster. I'm not sure how we could do it though.

Part of the challenge is that there is no guarantee that the type will be the same each time the pointer is used. It is possible that we could do some sort of caching for faster lookup, but we could punt.

Copy link
Member Author

Choose a reason for hiding this comment

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

I optimized the cases of slice index (32df15f) and structs (acf64f7).

I don't have any good ideas about optimizing other things.

@Keats Keats mentioned this pull request Jul 11, 2021
@Keats
Copy link

Keats commented Oct 13, 2021

@taiki-e is that still planned? Can I help?

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Overall, I'm definitely onboard with the concept! It would be nice if the documentation better described what this is for, though.

Also, I wonder if it would be worth adding a method like

pub trait Valuable {
   // ...

   fn get_pointer<'a>(&'a self, pointer: Pointer<'_>) -> Option<Value<'a>> {
      // ...
   }
}

that traverses a Pointer path and returns it as a Value if the pointed path exists in self?

This could probably have a default implementation (or be part of an extension trait, in order to make it totally impossible for users to override it).

valuable/src/pointer.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Oct 13, 2021

Another thought: in addition to having the macros (and possibly a Pointer::parse("x.y.z"), as suggested in #59 (comment)), I wonder if it would make sense to have a programmatic builder API for constructing pointer paths? Something like

/// `x[0].y`
let ptr = Pointer::builder()
   .field("x")
   .index(0)
   .field("y")
   .finish();

or similar?

But, that can probably be added in a follow-up branch as well.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@taiki-e
Copy link
Member Author

taiki-e commented Nov 23, 2021

Also, I wonder if it would be worth adding a method like

pub trait Valuable {
   // ...

   fn get_pointer<'a>(&'a self, pointer: Pointer<'_>) -> Option<Value<'a>> {
      // ...
   }
}

that traverses a Pointer path and returns it as a Value if the pointed path exists in self?

This could probably have a default implementation (or be part of an extension trait, in order to make it totally impossible for users to override it).

visit_pointer is a method for it: https://github.com/tokio-rs/valuable/pull/59/files#diff-a7abcaf3ded7efa976120acbbda5060e74513453ccbced80d88a443dc464110fR75-R78

@taiki-e taiki-e marked this pull request as ready for review November 23, 2021 12:30
valuable/src/pointer.rs Outdated Show resolved Hide resolved
use crate::{NamedValues, Slice, Valuable, Value, Visit};

/// A pointer to the value.
#[derive(Debug, Clone, Copy)]
Copy link
Member

Choose a reason for hiding this comment

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

it might be nice to have a Display formatter that formats the Pointer as an expression, like the inputs to the visit_pointer! macro. but, we could add that in a follow-up PR.

valuable/src/pointer.rs Outdated Show resolved Hide resolved
Comment on lines +41 to +47
#visit_pointer(
&#expr,
::valuable::pointer::Pointer::new(&[
#(#segments)*
]),
&mut #visit,
)
Copy link
Member

Choose a reason for hiding this comment

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

should we maybe also consider having a pointer! macro that returns a Pointer without actually trying to visit it? the use-case i have in mind is storing a pointer in a struct so that it can be use to traverse multiple Valuables.

Copy link

@Keats Keats Mar 9, 2022

Choose a reason for hiding this comment

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

+1 on that, in my case I would like to create the pointer when parsing the templates where I don't have the data structures yet

valuable/src/pointer.rs Outdated Show resolved Hide resolved
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