New render signature

[Evertt]

You can now render using any of these methods:

let html = // blabla some html-tags

print(html.render()) // this renders minified
print(html.render(indent: 2)) // this renders unminified with an indentation of 2 spaces per nesting-level

// or you can simply do:
print(html) // this renders minified again

There's two more things I'm considering to change:

1. The indent argument name that I'm using might not be clear enough, maybe it should be indentSize or usingIndentation. But I'm not sure if that adds more clarity and I don't want to use longer words if they don't add significantly more clarity.
2. I think it is possible to make the argument startingWithSpacesCount invisible / inaccessible from the outside and maybe that would be better, since we only use that argument on the inside. On the other hand, maybe people would like to be able to use that argument in certain edge cases.

[sahandnayebaziz]

Looks interesting to start. I left a few comments where I can't yet fully understand the code. After I understand and decide what to merge there, I'll look at the changes to render :)

[sahandnayebaziz]

How does this work with two parameters? It looks like we only need one, right? And it's optional? I don't mind the idea of collapsing the render functions, I just want to make sure that at the point of use this is easy to use.

To me, hidden/optional parameters can be a bit hard to find and see, whereas I think seeing multiple render functions pop up in autocomplete when you start typing ren.. is helpful and easy.

[Evertt]

@sahandnayebaziz the indent argument has the purpose to give the developer the choice how big they want their indentation to be. I usually write HTML with 4 spaces indentation and I saw that you chose 2 spaces, so I thought why not give the developer the choice.

I chose to make indent optional, because then you can convey you want a minified output by supplying nil for indent. Or not supplying it at all of course, because then nil is assumed.

Does that answer your question?

[sahandnayebaziz]

I think so. I'm going to load this version of Hypertext into a new package and see what it feels like with this naming.

[Evertt]

I also brought this branch up to date and fixed the tests here too...

[sahandnayebaziz]

@Evertt I am looking again at this PR now :)

[sahandnayebaziz]

Okay @Evertt I am glad you opened this pull request. I am playing with Hypertext now and I agree that public func render(startingWithSpacesCount: Int) -> String should let you pass your own indentation. I also love the CustomStringConvertible conformance. Also, looks like you've done some good general cleanup.

I'm happy to discuss all three with you on this pull request. If the conversation gets long, feel free to split into separate pull requests :) I'll start writing now...

[sahandnayebaziz]

Let's start with the indentation and the render signature. That's the biggest/best thing in here. Then after we can go to the rest in this PR :)

[sahandnayebaziz]

My intuition tells me that 99% of the time, Hypertext will be used just to generate HTML on a server to send back to a browser. And in that case of generating HTML for a server response, indentation doesn't matter. Right? All you need is valid markup. In fact, if it's not indented and formatted, it should be slightly smaller in size, plus the browser's inspector will format it and indent it for you anyway if you're developing the web application as well.

Uses I can think of for generating HTML that's formatted with indentation are maybe you're generating sample HTML code you want to show in code blocks, or maybe somebody writes a template engine and they want to write files to the file system that are pretty and human-readable. So I definitely think we should support it. I think somebody will find use in it.

So with these thoughts in mind, the "render with formatting" is the alternate case. So I would really like to keep two separate render functions in the Renderable protocol because, to me, it's nice and there is value in seeing this come up in Xcode:

One simple render() that takes no arguments and just works off of whatever is Renderable, be it a String, an Int, a tag, an array of tags, and anything you conform to Renderable. I think that's important. Anybody can use that all day to render, like maybe on a server, and then if you want to render with formatting, there is a second render function where you can (after this PR is merged) pass a number of spaces to indent by and then all the Renderables that are children know that's what you want to do and render themselves in an indented way however they are supposed to.

So can we keep the render() and render(formatting)?

[sahandnayebaziz]

Then, let's talk about the render with indentation and spaces count :)

[Evertt]

I'm confused. What are you asking for that isn't implemented in this PR? I understand that you like the idea of having a simple render() function available that renders minimized, because that is the most common use case probably.

But you're talking about it as if this PR doesn't offer that, but it DOES offer that:

[sahandnayebaziz]

This PR does offer that but by way of an extension to Renderable that takes an optional argument

public protocol Renderable: CustomStringConvertible {
    func render(indent: Int?, startingWithSpacesCount: Int) -> String
}

public extension Renderable {
    ...

    func render(indent: Int? = nil) -> String {
        return render(indent: indent, startingWithSpacesCount: 0)
    }
}

Sorry for being confusing, I just wanted to explain my thinking for you first before asking for something :)

I am asking for us to keep Renderable as:

public protocol Renderable {
  func render() -> String
  func render(startingWithSpacesCount: Int) -> String // But let's work on this one and change it to take the space/indent like you have
}

[sahandnayebaziz]

My goals are to:

• make the autocomplete that pops up on the end of a Renderable show only render() and our second render(indent/startingWithSpacesCount)
• set Renderable up so that making a custom type conform to Renderable means you implement render() which is the basic, nothing-added output of your instance to HTML, and then also implement render(indent/startingWithSpacesCount) so that you specifically consider the case where formatting is happening.

For example, if somebody has a tag that is some sort of document comment, maybe they always want that to be at the very beginning of a line all the way to the side with no indentation, so in the formatting render they can specifically ignore any spacing/indentation. Or if they have some type that's sort of like an array that carries items in it, they can iterate through their items in the formatting render and add their own nested indents.

[sahandnayebaziz]

and as for the name, I like that put indent in there. I'm playing with Hypertext now and I think swapping the order of spaces/indent makes more sense. Right? Because first you get all the spaces that this Renderable is inheriting from the formatting of previous parents/siblings, and then you see what the requested indentation is that can be added.

Something like

...

  func render() -> String
  func render(startingWithSpacesCount: Int, indentingWithSpacesCount: Int) -> String

...

We can remove the default for indentingWithSpacesCount. What do you think? To me, we know little about what cases somebody would want to generate indented HTML in and so we can leave that explicit the first you call it. Also, this will keep the autocomplete options users see tidy.

[Evertt]

So I'm hearing you say a few things:

1. You'd like the simple render() method to be part of the protocol, because:
   1. You'd like the autocomplete to be completely clean and only show the two render functions
   2. If it's part of the protocol (and not (just) the extension) people can overwrite it and provide their own implementation.
   3. If there's a dedicated method for minified rendering, then the indent argument no longer needs to be nullable
2. You'd like to swap the order of the spaces / indent, because it makes more sense to you that way.

Did I get you right? I totally agree with 1.i and 1.ii and I have some objections towards the rest of your philosophy.

I really value ease / simplicity and clarity at the call site. Like you said, I imagine that there are two main use cases for using this function:

1. On a website, where minified rendering is always preferred, this will be most common.
2. During debugging, where indented rendering is always preferred.

When I look at those use cases I think nobody really cares about being able to manipulate how the indentation will work. They'll just want it to work. Indented rendering for debugging purposes doesn't need to have a certain style, it just needs to be readable.

So this is what I did in the changes that I just committed:

• I kept the indent argument as an optional, because that just makes the code of the default implementations so much cleaner.
• I did put the render() method back into the protocol so people can implement their own version if they really want to, but I also kept it in the extension so people don't have to implement their own version.
• I changed the render method in such a way so that startingWithSpacesCount has become obsolete. This way the autocomplete stays cleaner, because that argument had no value at the call site at all.

So to summarize: I really want to keep things simple and easy for the developers using our library. I want a developer that implements their own custom tag to not have to worry about implementing a bunch of (render) methods that they don't care so much about anyway. And I want a developer that calls the render methods to not be distracted by arguments that are meaningless in that context, like startingWithSpacesCount.

Would you like to review my changes and tell me what you think?

[sahandnayebaziz]

I am glad you see 1.i and 1.ii. Thank you for reading and listening. I think I agree with the rest of your philosophy. I am beginning to lose the bigger picture over the past couple changes, so I'm going to load this file up locally and play with it and I will write back :)

[Evertt]

Whoops, I though I fixed the tests, but apparently not. Give me a sec..

[sahandnayebaziz]

Take your time. I see what you're doing here, and I like it, but I think I want to do it a little bit differently. I still want you to have the credit on this pull request though, so after you fix the tests I will stop trying to explain what I'm thinking and just put it into code and paste it as a comment onto this PR so you can see :)

[sahandnayebaziz]

Or, if you'd rather, don't worry about the tests and we can fix those when this PR is ready, which hopefully they will fix themselves :)

[sahandnayebaziz]

Sweet! I see you fixed the tests

[Evertt]

Did you read my reply here? I'd like to know what you think about it.

[sahandnayebaziz]

@Evertt just read, replying in a new comment :)

[sahandnayebaziz]

@Evertt I'm going to split this PR up into separate PRs so we can merge the things that are already great (like CustomStringConvertible) and then discuss the other parts :)

Especially after re-reading your original pull request description :)

[sahandnayebaziz]

@Evertt I made the first split in #17 :) Going to merge that in now since we both seem pretty fine with that.

[sahandnayebaziz]

Okay @Evertt I see one problem with removing startingWithSpacesCount in favor of using indent() (which I think is very clever!).

With startingWithSpacesCount (and a default of 2 spaces or a passed in Int for the indent number) each Renderable is always given the number of spaces that are currently being requested as starting (and the number for the indent that the user wants). So most Renderables, like all of the default tags, will respect the starting spaces count and then add spaces for indent.

With a custom Renderable, when you account for the normal render output and unminified, you specify each. If your Renderable has a simple output, you can shortcut by implementing render(withStart...) as just render() with spaces+indent at the front. Or, you can choose to ignore the number of starting spaces and do something like always be in the string on its new line at the very beginning, or maybe always at some set number of spaces. But this way all tag subclasses know what to do and then if you want to implement something like a HTML comment or some sort of document marker that should always be at the beginning, or some other templating thing, you can do it because each Renderable knows how to render plainly and how to render when given a certain spaces being used and certain indent.

But if we switch to indent(), then every tag automatically indents its children. So the parent Renderable is do something to the children whereas if each Renderable gets the startingSpaces and the indent, each one can decide what it wants to do AND decide whether or not to increment the startingSpaces when passing that value onto its children.

So even though it's shorter to get rid of indent and startingSpaces, I think it makes it better if we keep both and pass them along. This way the person who writes the Renderable can decide what to do with the knowledge of both.

[sahandnayebaziz]

And if it's not clear, I do agree that we should get rid of the default 2 and make it an int that you pass in, because why not. Might as well make that open. I expect that render to be used in very specific cases where the user probably knows exactly what they want.

[Evertt]

@sahandnayebaziz okay I think I agree with you. Still I kind of find the startingWithSpacesCount a bit ugly when it appears in autocomplete at the call site. However, I think I found a way around that and I'd like to show it to you.

I'll commit my changes shortly. Also, I found a better way to convey your intention about minifying or indenting without needing an optional. I'll also show that in the next commit.

[Evertt]

Okay, it's done. So here's what changed:

• Instead of a nullable indent argument, I'm using a new enum type with which you can convey your intention. So you can render using these four options:

  print(document)
  print(document.render())
  print(document.render(.minified))
  print(document.render(.indented(spaces: 2)))

  I think that is much more clear and readable.

• Now I also made it possible for every tag to decide whether they want to conform to the indentation or not. If you want to make a tag that does not conform to the normal indentation, you can define it like this:

  public class blabla: tag {
      override func render(_ mode: RenderMode, startWithSpaces count: Int) -> String {
          return super.render(mode, startWithSpaces: 0)
      }
  }

• I was able to make that all possible while keeping the autocomplete really clean:
  

The only downside is that the code of the library is a bit more messy now. But I think that's worth it for how much power and beauty it gives to the developers who will use this library.

[Evertt]

Aw shiiiiiit, I just realized that this is not as perfect as I thought it was. I made the protocol InternalRenderable internal, as the name implies, so that you don't have to see its methods in autocomplete. However, that also means you can't override it. You can only override Renderable (which doesn't give you the same power over the indentation) or tag (which implies your making a new tag, which isn't always the case. If I make InternalRenderable public then all of its methods will also have to be visible to the public (and so they will pop up in autocomplete)...

So right now, the only way to make an html-comment for example is by doing this:

public class comment : tag {
    public override var name: String {
        return "!--"
    }
}

That produces a technically valid html comment, but it will look a bit strange. To make it look more natural, you'd have to do this:

public class comment : tag {
    override func render(_ mode: RenderMode, startWithSpaces count: Int) -> String {
        guard let children = children else {
            return "<!-- -->".indent(spaces: count)
        }

        let content = children.render(mode, startWithSpaces: mode.addSpaces(to: count))

        switch mode {
        case .minified:
            return "<!-- \(content) -->"
        case .indented:
            let open = "<!--".indent(spaces: count)
            let close = "  -->".indent(spaces: count)

            return "\(open)\n\(content)\n\(close)"
        }
    }
}

[sahandnayebaziz]

@Evertt this is interesting... I like where your head is at. But I agree, I do think this makes the library more complicated and I don't love it like I love the other work we've done.

Tell you what, let's get the original indentation part of this pull request in and then revisit other parts of this in the future after 2.0. 2.0 is looking really nice with everything you've done.

So which do you prefer at the call site...

func render(startingWithSpaces: Int, usingIndentation: Int) -> String

or

func render(startingWithSpacesCount: Int, indentingWithSpacesCount: Int) -> String

[Evertt]

I prefer

func render(startingWithSpaces: Int, indentingWithSpaces: Int) -> String

Because the count is already implied by the fact that we're asking for an integer. And one my favorite rules of the swift api design guidelines is the one about omitting needless words.

By the way, my other favorite rule is the one about making code read as plain English.

[sahandnayebaziz]

Those API design guidelines are awesome. I had not read them in a while. Thank you for reminding me of them.

I have merged the new indentation signature and all of the other changes and released 2.0.0! :)

Thank you @Evertt