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

feat(lib): Implement Jekyll's version of sort #438

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

Conversation

epage
Copy link
Member

@epage epage commented Mar 20, 2021

Part of #311

@epage
Copy link
Member Author

epage commented Mar 20, 2021

@limeschool : posting this to keep you updated.

@ghost
Copy link

ghost commented Mar 20, 2021

Thanks for working on this, @epage! If I was actually capable of writing decent code, I would've made a PR myself.

Apologies if I came off as whiny earlier, and thanks, really.

@epage
Copy link
Member Author

epage commented Mar 20, 2021

Want to finish this? I'm trying to wrap up another project before I focus on liquid/cobalt (so I can better focus).

The main other thing to know is the function try_find

@ghost
Copy link

ghost commented Mar 20, 2021

I'm working on other things right now myself, but I will give it a looking at once I learn how to write tests (I'm really amateur, haha) and once I've looked at how everything works in this crate.

@epage
Copy link
Member Author

epage commented Mar 20, 2021

but I will give it a looking at once I learn how to write tests (I'm really amateur, haha)

The PR includes some of Jekyll's tests being ported with Rust with the remaining ones partially ported and commented out. Comparing the original tests with the ones that I already ported should get you most of the way there.

As for the other parts, the surrounding glue is written, just the implementation needs writing. As a hint`

let args = self.args.evaluate(runtime)?;

takes SortArgs and constructs a derive-constructed struct that has property: Option<str>, nils: Option<str>. So you can just do args.property and args.nils.

@emmyoh
Copy link

emmyoh commented May 31, 2024

@epage I have something like this:

fn safe_property_getter<'a>(value: &'a Value, property: String) -> Value {
    value
        .as_object()
        .and_then(move |obj| {
            if let Some(scalar) = obj.as_scalar() {
                try_find(&property.clone(), &vec![scalar]).map(|v| v.clone().to_value())
            } else {
                obj.get(&property).map(|v| v.to_value())
            }
        })
        .unwrap_or(Value::Nil)
}

It can get a top-level property, but not anything deeper (ie, it fails when property is of the form XXX.YYY).
Have you figured out how to get nested properties of an ObjectView?


EDIT: This seems to work:

fn safe_property_getter<'a>(value: &'a Value, property: String) -> Value {
    let properties = property.split('.').collect::<Vec<&str>>();
    let mut result = value.to_owned();
    for property in properties {
        result = result
        .as_object()
        .and_then(move |obj| {
            if let Some(scalar) = obj.as_scalar() {
                try_find(&property, &vec![scalar]).map(|v| v.clone().to_value())
            } else {
                obj.get(&property).map(|v| v.to_value())
            }
        })
        .unwrap_or(Value::Nil);
    }
    result
}

Changes are here: https://github.com/epage/liquid-rust/compare/sort...emmyoh:liquid-rust:jekyll-sort?expand=1.

@emmyoh
Copy link

emmyoh commented May 31, 2024

Down to two failing tests:

  • raise_exception_when_input_is_nil
  • return_sorted_by_property_array_with_nils_first

If you have the time, would you mind taking a look?


EDIT: Figured it out. New commit pushed to my branch where all tests now pass.

@@ -117,7 +117,7 @@
}

/// Find a `ValueView` nested in an `ObjectView`
pub fn try_find<'o>(value: &'o dyn ValueView, path: &[ScalarCow<'_>]) -> Option<ValueCow<'o>> {
pub fn try_find<'o, 'p>(value: &'o dyn ValueView, path: &[ScalarCow<'p>]) -> Option<ValueCow<'o>> {

Check failure

Code scanning / clippy

the following explicit lifetimes could be elided: 'p Error

the following explicit lifetimes could be elided: 'p
@@ -117,7 +117,7 @@
}

/// Find a `ValueView` nested in an `ObjectView`
pub fn try_find<'o>(value: &'o dyn ValueView, path: &[ScalarCow<'_>]) -> Option<ValueCow<'o>> {
pub fn try_find<'o, 'p>(value: &'o dyn ValueView, path: &[ScalarCow<'p>]) -> Option<ValueCow<'o>> {

Check failure

Code scanning / clippy

the following explicit lifetimes could be elided: 'p Error

the following explicit lifetimes could be elided: 'p
@@ -117,7 +117,7 @@
}

/// Find a `ValueView` nested in an `ObjectView`
pub fn try_find<'o>(value: &'o dyn ValueView, path: &[ScalarCow<'_>]) -> Option<ValueCow<'o>> {
pub fn try_find<'o, 'p>(value: &'o dyn ValueView, path: &[ScalarCow<'p>]) -> Option<ValueCow<'o>> {

Check failure

Code scanning / clippy

the following explicit lifetimes could be elided: 'p Error

the following explicit lifetimes could be elided: 'p
@@ -117,7 +117,7 @@
}

/// Find a `ValueView` nested in an `ObjectView`
pub fn try_find<'o>(value: &'o dyn ValueView, path: &[ScalarCow<'_>]) -> Option<ValueCow<'o>> {
pub fn try_find<'o, 'p>(value: &'o dyn ValueView, path: &[ScalarCow<'p>]) -> Option<ValueCow<'o>> {

Check failure

Code scanning / clippy

the following explicit lifetimes could be elided: 'p Error

the following explicit lifetimes could be elided: 'p
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