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

Result builder fixes #137

Merged
merged 5 commits into from
Jul 8, 2023
Merged

Conversation

tonyarnold
Copy link
Contributor

This PR proposes to update the ContentBuilder to support partial blocks. This seems to resolve the issues that I was seeing with #132, and potentially other unexpectedly invalid content combinations.

Sorry for the lack of comment documentation on these revised methods.

I've also included conformance of the Link type to BodyElement as it's valid HTML.

@mattesmohr
Copy link
Member

mattesmohr commented Jul 2, 2023

Haha, I was looking into this yesterday too. But you seem to be faster than me. Tests are running fine, so I think it's good to go.

I will test it on my sites and see if something is coming to my attention.

@mattesmohr
Copy link
Member

mattesmohr commented Jul 2, 2023

My sites are doing fine with your result builder. Great job!

I do not understand every line of your code yet, but I want to, before I merge it, if that's okay?

What were the reasons for removing the comments? Not helpful enough?

@mattesmohr
Copy link
Member

mattesmohr commented Jul 2, 2023

I would add those tests to the StatementTests too, so we can be sure we take your cases into account for future changes.

    func testOptionalAfterElement() throws {
        
        let name: String? = "Tony"
        
        let view = TestView {
            Division {
            }
            if let name = name {
                Paragraph {
                    name
                }
            }
        }
        
        XCTAssertEqual(try renderer.render(view: view),
                       """
                       <div></div>\
                       <p>Tony</p>
                       """
        )
    }
    
    func testOptionalBeforeElement() throws {
        
        let name: String? = "Tony"
        
        let view = TestView {
            if let name = name {
                Paragraph {
                    name
                }
            }
            Division {
            }
        }
        
        XCTAssertEqual(try renderer.render(view: view),
                       """
                       <p>Tony</p>\
                       <div></div>
                       """
        )
    }
    
    func testOptionalWithExpectedResult() throws {
        
        let name: String? = "Tony"
        
        let view = TestView {
            Body {
                if let name = name {
                    Paragraph {
                        name
                    }
                }
            }
        }
        
        XCTAssertEqual(try renderer.render(view: view),
                       """
                       <body>\
                       <p>Tony</p>\
                       </body>
                       """
        )
    }
``

@tonyarnold
Copy link
Contributor Author

I do not understand every line of your code yet, but I want to, before I merge it, if that's okay?

Of course, take your time!

What were the reasons for removing the comments? Not helpful enough?

I started over, and was doing it at the dinner table with my kids underfoot. I honestly didn't have time to test each method in the result builder this evening, so I just sent it through as a PR as-is.

I think the comments should be added before merging this — I just don't have time to do it this evening :)

@mattesmohr
Copy link
Member

No problem. I hope it's okay, I intervene/interfere in your PR. I have added some real quick, because I do not know if I have time for it tomorrow.

@mattesmohr
Copy link
Member

mattesmohr commented Jul 3, 2023

I could break it down to those few functions. Can you check if it works for you too, before I push it?

// Abstract:
// The file contains the builder to build up the result from a sequence of elements.

/// The builder builds up a result value from a sequence of elements.
@resultBuilder public enum ContentBuilder<T> {
    
    public typealias Component = [T]
    
    public typealias Expression = T

    /// Builds an empty block
    ///
    /// ```swift
    /// Tag {
    /// }
    /// ```
    public static func buildBlock() -> Component {
        return []
    }
    
    /// Builds a block with one element.
    ///
    /// ```swift
    /// Tag {
    ///    Tag {
    ///    }
    /// }
    /// ```
    public static func buildPartialBlock(first: Component) -> Component {
        return first
    }

    /// Builds a block with more than one element.
    ///
    /// ```swift
    /// Tag {
    ///    Tag {
    ///    }
    ///    Tag {
    ///    }
    /// }
    /// ```
    public static func buildPartialBlock(accumulated: Component, next: Component) -> Component {
        return accumulated + next
    }

    /// Builds a block
    ///
    /// ```swift
    /// let content = "Content"
    ///
    /// Tag {
    ///    content
    /// }
    /// ```
    public static func buildExpression(_ element: Expression?) -> Component {
        
        guard let element else {
            return []
        }
        return [element]
    }
    
    /// Builds a block
    ///
    /// ```swift
    /// let content: [Type]
    ///
    /// Tag {
    ///    content
    /// }
    /// ```
    public static func buildExpression(_ element: Component) -> Component {
        return element
    }

    /// Builds a block with one element.
    ///
    /// ```swift
    /// Tag {
    ///    if let unwrapped = optional {
    ///       Tag {
    ///          unwrapped
    ///       }
    ///    }
    /// }
    /// ```
    public static func buildOptional(_ component: Component?) -> Component {

        guard let component else {
            return []
        }
        return component
    }

    /// Builds a block, if the condition is true.
    ///
    /// ```swift
    /// Tag {
    ///    if(true) {
    ///       Tag {
    ///       }
    ///    }
    /// }
    /// ```
    public static func buildEither(first: Component) -> Component {
        return first
    }

    /// Builds a block, if the condition is false.
    ///
    /// ```swift
    /// Tag {
    ///    if(false) {
    ///       Tag {
    ///       }
    ///    }
    ///    else {
    ///       Tag {
    ///       }
    ///    }
    /// }
    /// ```
    public static func buildEither(second: Component) -> Component {
        return second
    }

    /// Builds blocks by running through a sequence of elements.
    ///
    /// ```swift
    /// Tag {
    ///    for element in sequence {
    ///       Tag {
    ///          element
    ///       }
    ///    }
    /// }
    /// ```
    public static func buildArray(_ components: [Component]) -> Component {
        return components.flatMap { $0 }
    }
}

@mattesmohr
Copy link
Member

@tonyarnold Hiya, could you find some time to copy paste the result builder I have posted into your project to see if it works for you too?

@tonyarnold
Copy link
Contributor Author

tonyarnold commented Jul 8, 2023

Hi @mattesmohr! I've just tried your suggested amendments, and they seem to work for me, too - no compilation errors, and everything seems to be rendering as it did with my earlier revision.

@tonyarnold
Copy link
Contributor Author

Would you like me to update this PR with those changes, or are you happy to do that?

Copy link
Member

@mattesmohr mattesmohr left a comment

Choose a reason for hiding this comment

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

Awesome! Thank your for testing. Don't bother. Consider it as done.

@mattesmohr mattesmohr merged commit 8f253e7 into vapor-community:main Jul 8, 2023
1 check passed
@tonyarnold tonyarnold deleted the additions branch July 8, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants