huboard / huboard-web

GitHub issues made awesome
https://huboard.com
61 stars 26 forks source link

Adds a composable client-side parser in prep for Card Relationships #302

Closed discorick closed 8 years ago

discorick commented 8 years ago

Allows for the building of a parse tree from an arbitrary input (so long as selected parser allows that input) I.E

import cardRelationshipParser from 'app/utilities/parsing/card-relationship-parser'

var tree = cardRelationshipParser.parse('some kind of input') // returns a tree
console.log(tree)

{
   'issue-references': ['#256', 'somerepo/#422'],
   'parent-issues': ['#122']
}
dahlbyk commented 8 years ago

Nothing really to add....where do we go from here?

discorick commented 8 years ago

Just wanted to make sure your cool with the direction of the design, but I could implement the logic for parsing issue references, or merge as is and defer that to another PR.

dahlbyk commented 8 years ago

Direction is legit. I'd rename this and keep going on building out CardRelationshipParser to see if the design continues to work well as we try to do real stuff with it. Shipping in isolation doesn't really make sense since we haven't really tried to build anything real with it.

discorick commented 8 years ago

I pushed up a working implementation of the Issue-Reference rule, the CardRelationshipParser is now capable of discovering issue references.

dahlbyk commented 8 years ago

Thoughts on pulling out the href rather than the link text? Thinking comment links, e.g. https://github.com/huboard/huboard-web/pull/302#issuecomment-220667861.

discorick commented 8 years ago

Thoughts on pulling out the href rather than the link text?

It's possible, the downside whatever piece of the app performs actions on the parse tree will just that much more logic to sort through an jQuery objects. I am not sure how the extra data helps us, as we will be matching up the parsed issues later to their actual models anyways.

When we build a CommentReference rule that could be a different story though.

What do you think?

dahlbyk commented 8 years ago

My concern is that the current implementation will also match comment links:

<a title="Adds a composable client-side parser in prep for Card Relationships" href="https://github.com/huboard/huboard-web/pull/302#issuecomment-220667861" class="issue-link js-issue-link" data-id="155374764" data-error-text="Failed to load issue title" data-permission-text="Issue title is private">#302 (comment)</a>

Which will add #302 (comment) to the array. Is that what we want?

discorick commented 8 years ago

Oh, I misunderstood the question, good point, I will add a test case for that.

discorick commented 8 years ago

Which will add #302 (comment) to the array. Is that what we want?

This is an interesting question, it would be simple enough to stripe the comment references out - or simply leave them intact. They match the criteria of a reference even if we don't have plans for UX to support them.

My current thought is to ignore any references matching /\(comment\)/ in the body.

rauhryan commented 8 years ago

My current thought is to ignore any references matching /(comment)/ in the body.

I think we would be much better off parsing out the data-id attribute or break it out into a model that has the URL and the id

<a href="https://github.com/huboard/huboard-web/pull/302#issuecomment-220667861" class="issue-link js-issue-link" data-id="155374764" data-error-text="Failed to load issue title" data-permission-text="Issue title is private" >#302 (comment)</a>
discorick commented 8 years ago

think we would be much better off parsing out the data-id attribute or break it out into a model that has the URL and the id

Thats an option, the parse tree could look like something like this for issue-references:

{
   'issue-references': [
      {
        id: 155374764 ,
        url: 'https://github.com/huboard/huboard-web/pull/302#issuecomment-220667861'
        body: '#302 (comment)'
    },
    {
      id: 123456789 ,
      url: 'https://github.com/huboard/huboard-web/issues/111',
      body: '#111'
    },
}

Then later, after the board model is loaded, we can use the id keys to match the backing issue data with it's reference.

dahlbyk commented 8 years ago

I do think we want to extract both. id is the simplest lookup for issues that are on the board, but if the issue isn't on the board, we may eventually wish to fetch its details from GitHub (which we can't do by id AFAIK).

discorick commented 8 years ago

So thats a thumbs up on the objects?

dahlbyk commented 8 years ago

So thats a thumbs up on the objects?

Aye, tho maybe text instead of body?