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

chore: pre-release checklist #1

Open
6 tasks
jonschlinkert opened this issue Nov 25, 2018 · 24 comments
Open
6 tasks

chore: pre-release checklist #1

jonschlinkert opened this issue Nov 25, 2018 · 24 comments

Comments

@jonschlinkert
Copy link
Member

jonschlinkert commented Nov 25, 2018

  • port remaining Ruby filters to javascript
  • create all of the filters mentioned in the Shopify docs (docs are in markdown here, for easy access)
  • port Jekyll/Liquid "tags" to javascript (when possible and practical. these are similar to handlebars block helpers)
  • get rid of most deps
  • remove duplicate filters (I created a couple of different versions of some filters, so we can discuss which direction we want to go. We can continue with this pattern until we're ready to publish, then we can remove dupes).
  • write unit tests for all filters

Tests

There are other javascript ports of liquid, we can probably get more tests and filters from those as well.

cc @tunnckoCore @doowb

@jonschlinkert
Copy link
Member Author

@tunnckoCore how do you want to divide up the tasks? Do you want to do unit tests? Or convert filters? Or both?

Or we can each pick a few categories of filters to work on.

@tunnckoCore
Copy link
Collaborator

I can handle the tests and few categories.

We can continue with this pattern until we're ready to publish, then we can remove dupes

Ookey.

@jonschlinkert
Copy link
Member Author

I can handle the tests and few categories.

Awesome! Let me know which categories you're going to do so I don't do the same ones. thanks!

@tunnckoCore
Copy link
Collaborator

The color and money ones.

@jonschlinkert
Copy link
Member Author

sounds good!

@jonschlinkert
Copy link
Member Author

@tunnckoCore if you remember, try to put the methods in alphabetical order so we can more easily scan for missing/duplicate filters. Not a big deal.

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Nov 26, 2018

Yea, was thinking about that too. Btw, should we preserve the underscore naming? I don't think so, because we are in the JavaScript land and linters may signal errors if users follow camelcase or other convention.

@jonschlinkert
Copy link
Member Author

jonschlinkert commented Nov 26, 2018

Btw, should we preserve the underscore naming?

Yeah, let's keep the underscores. I know it sucks, I agree - I don't like them personally either. My main goal is to make it really easy for Jekyll users to switch to another framework. We can easily create aliases for those though.

Edit: also it will make it a lot easier for us to scan the filters on liquid libs and check for parity.

@jonschlinkert
Copy link
Member Author

linters may signal errors if users follow camelcase or other convention.

We can figure this out before we publish. Maybe we'll create a "name mapper" for aliases, so we can use camelcase, but for now IMHO it will make this easier if we stick to the same names.

@jonschlinkert
Copy link
Member Author

One more thing... I'm trying to make most of the filters generic, but there are a few that have options.hash in them, which is a handlebars syntax. The reason is that those were ported from liquid "tags", which are more like handlebars block helpers, and there is no good way to port those to single line helpers that are template-engine-agnostic. We need to figure out a good way to expose those, maybe on a block property, or tags or something, but for now it's something to think about.

@jonschlinkert
Copy link
Member Author

@tunnckoCore I just pushed up. Hopefully you don't get any conflicts, I didn't touch tests or the filters you're working on (do you get notifications when someone pushes up to a repository you collaborate on?)

@tunnckoCore
Copy link
Collaborator

No, everything is good. I'm working on a branch too ;d

do you get notifications

Yea, it's not a problem to me, I always have tons of notif cuz I watch tons of repos.

@jonschlinkert
Copy link
Member Author

jonschlinkert commented Nov 26, 2018

K, here is an example of how I'm doing comments. Don't worry too much about these, I can edit:

/**
 * Truncate a string to the specified `length`, and append
 * it with an elipsis, `…`.
 *
 * ```html
 * {{ "<span>foo bar baz</span>" | ellipsis: 7 }}
 * <!-- Results in: 'foo bar…' -->
 * ```
 * @param  {String} str
 * @param  {Number} length The desired length of the returned string.
 * @param  {String} suffix (optional) Custom characters to append. Default is `…`.
 * @return {String} The truncated string.
 * @api public
 */

exports.ellipsis = (str, len, suffix) => {
  return isString(str) ? (exports.truncate(str, len) + (suffix || '…')) : '';
};

I'm debating whether or not we should do the code examples in the comments, or if we should do them separately in docs.

Several years ago I created a code comment linter that reported how many methods were undocumented and a few other useful stats. IMHO that might be a good solution here. I have something we can use if we want to go this route.

edit: it would be awesome if our docs had examples for several template engines, but comments like the following are just too verbose and hard to maintain:

/**
 * Pads the given string with another string until the resulting
 * string reaches the specified maxiumum length. The padding is
 * applied from the end (right) of the current string.
 *
 * ```js
 * <!-- Signature: string.pad_end(str, maxLength[, padString]) -->
 * Liquid:     {{ "Foo" | pad_end: 10, "0" }}
 * Handlebars: {{pad_end "Foo" 10 "0"}}
 * ejs:        <%= pad_end("Foo", 10, "0") %>
 * <!-- Results in: 'Foo0000000' -->
 * ```
 * @param {String} str
 * @param {String} maxLength
 * @param {String} padString
 * @return {String}
 * @api public
 */

I have an idea... I'll create another issue.

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Nov 26, 2018

What about just using multiple @examples?

/**
 * Pads the given string with another string until the resulting
 * string reaches the specified maxiumum length. The padding is
 * applied from the end (right) of the current string.
 *
 * @example hbs
 * <h1>{{ pad_end "Foo" 10 "0" }}</h1>
 *
 * @example liquid
 * <h1>{{ "Foo" | pad_end: 10, "0" }}</h1>
 *
 * @example ejs
 * <h1><%= pad_end("Foo", 10, "0") %></h1>
 *
 * @param {String} str
 * @param {String} maxLength
 * @param {String} padString
 * @return {String}
 * @public
 */

It not looks that bath bad, both in code and in docs.

@jonschlinkert
Copy link
Member Author

jonschlinkert commented Nov 26, 2018 via email

@tunnckoCore
Copy link
Collaborator

And what? I noticed that you are not fan (in the tokenize-comment), but why? it has great syntax highlighting support everywhere. And completely make sense.

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Nov 26, 2018

Should we throw from the helpers or just to return an empty string?

@jonschlinkert
Copy link
Member Author

jonschlinkert commented Nov 26, 2018

Should we throw from the helpers or just to return an empty string?

The convention with most template engines is to just return an empty string. I assume this is because it's better than having undefined in your generated HTML and/or requiring the user to write conditional code in templates to populate an empty string when the value is undefined.

Honestly, I've never felt good about either option here. What are your thoughts? @doowb?

And what? I noticed that you are not fan (in the tokenize-comment), but why? it has great syntax highlighting support everywhere. And completely make sense.

I'm not sure what you mean. tokenize-comment parses those completely, even when there are multiple of them. I just don't like using them in my own comments, I personally find them harder to read. Using very light markdown syntax makes the comments formatted more similarly to the actual documentation used in markdown, which makes it easier for the user to glance at the code and figure out what's going on. just my 2c.

@jonschlinkert
Copy link
Member Author

I just added more javadoc tests so you can see what I mean. Let me know if I'm not understanding what you mean.

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Nov 26, 2018

Honestly, I've never felt good about either option here.

I think they should still throw but the template engine to handle that cases, so users will immediately understand what happens. It is always better to inform the user instead of giving him some undefined or empty strings.

I'm not sure what you mean. tokenize-comment parses those completely, even when there are multiple of them.

Yea yea, I know. I got your point, it's personal preference.

Using very light markdown syntax makes the comments formatted more similarly to the actual documentation

Actually no. 😆 It seems weird. Because in the api docs they see the actual example code, syntax highlighted, but in the code comments they see some gfm fence block, which isn't highlighted... Yes, with @example it still isn't highlighted, but at least you immediately see where the code example starts and ends. Only because the @example is marked as tag.

@jonschlinkert
Copy link
Member Author

Actually no. 😆 It seems weird. Because in the api docs they see the actual example code, syntax highlighted, but in the code comments they see some gfm fence block, which isn't highlighted.

So, to be clear, you are saying that this "seems weird"

image

which renders to

image

because... even though it's identical to what's in the code comment, the @example tag, which looks like this:

image

...makes it easier to see where the example begins and ends? You think that's more obvious than ```?

Okay, I'm totally cool with people having different opinions, and it's 100% fine if you just prefer @example for whatever reason. But there is no universe in which that is more clear. We'll have to agree to disagree on this one lol.

@jonschlinkert
Copy link
Member Author

Honestly, the reason I don't like @example is that it always looks like someone commented out code and forgot to remove it, until I do a double take.

@tunnckoCore
Copy link
Collaborator

tunnckoCore commented Nov 26, 2018

I don't think indentation for the @example tag is needed? Also it seems like your editor theme do not highlight the tags?

2018-11-26-105331_1280x1024_scrot

Isn't it obvious where the example end? 😆

Which renders absolutely the same as in the code comment...

2018-11-26-105545_1280x1024_scrot

@jonschlinkert
Copy link
Member Author

Also it seems like your editor theme do not highlight the tags?

Haha, I disable that. I don't like when code comments are colored similar to the actual code. It's just my preference. I find it distracting.

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

No branches or pull requests

2 participants