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

Design formatter API to be more broad #318

Open
yamikuronue opened this issue Jun 3, 2016 · 7 comments
Open

Design formatter API to be more broad #318

yamikuronue opened this issue Jun 3, 2016 · 7 comments

Comments

@yamikuronue
Copy link
Contributor

The current formatter arguments are nodeBB specific, like URLForPost only taking a post ID (most forums require a thread ID as well).

@AccaliaDeElementia
Copy link
Member

okay, looking at this issue, i'm not sure of the long term suitability of the urlFor*() functions.

Basically, each provider would need a different set of parameters to the functions. nodebb and discourse have redirectors for postId, and both accept topicId/slug/postindex for topics, but something like phpbb would need topicId/page number/anchor for its urls... and something like a slack provider or an irc provider would have no concept of a url to a post.

too much can change from provider to provider to give the urlFor*() functions consistent and logical signatures...

I think the better approach there is to go with the approach already used in the provider for the objects. each has a url() -> Promise<string> function that generates the url as applicable.

@yamikuronue, your thoughts?

See Post.url() for an example

@yamikuronue
Copy link
Contributor Author

yamikuronue commented Jun 27, 2016

That's a great idea :)

I think we should do a Formatter.linkTo(target) though, where Target is a Post or a Topic or a User. Since they all have a .url() method, it can call that method, and then wrap it in a link format. If the target is a string, it should just put that into the link format, assuming it's a valid url already.

How about the other formatter methods?

@AccaliaDeElementia
Copy link
Member

hmm.... i think the other ones are pretty agnostic. it might make sense to add support to them to have a "url-able" passed in instead of a string for urls for image/link/quote /etc. but to do so would be a breaking change. right now all the formatters are synchronous, and the url() function returns a Promise<string>

i'll have to think on that one.

but for now at least i think we're agreed on deprecate the urlFor*() functions and remove at a later date?

@yamikuronue
Copy link
Contributor Author

yeah

@yamikuronue
Copy link
Contributor Author

Is this done?

@AccaliaDeElementia
Copy link
Member

i don't think there's been much traction on this one to be honest.

it's something we need to look at and deal with "soon(tm)" but it's also one of those boring mainentance things so i've been dragging my paws on it, you know? :-)

@yamikuronue
Copy link
Contributor Author

The URL function is everywhere already, so it's just a formatter change. I may tackle this soon then, I'm very familiar with formatter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants