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

"send-an-email-on-DLC-by-midnight" became "Send an email" #191

Closed
lithp opened this issue May 10, 2018 · 9 comments
Closed

"send-an-email-on-DLC-by-midnight" became "Send an email" #191

lithp opened this issue May 10, 2018 · 9 comments
Labels
ADO Consensus needed on what to Actually Do (or "much ado about ∅"), AKA question BUG dup (closed as) duplicate UVI User-Visible Improvement
Projects

Comments

@lithp
Copy link
Contributor

lithp commented May 10, 2018

When I typed the url http://brian.commits.to/send-an-email-on-DLC-by-midnight the Title became Send on email, instead of my intended Send an email on DLC. I'm guessing it's because "on" was parsed as a date and truncated from the title, even though some subsequent code-path failed to recognize "DLC" as a date?

@lithp
Copy link
Contributor Author

lithp commented May 10, 2018

This might be related to #164

@lithp
Copy link
Contributor Author

lithp commented May 10, 2018

Yeah, this is a duplicate of #164. Leaving this open because I think it's still an issue, this is a bad user experience! Now I have to be afraid that my descriptions will be wrong and check them every time, that seems contrary to the idea of "commits.to is fast and easy: just type a url and you're done".

@lithp
Copy link
Contributor Author

lithp commented May 16, 2018

This is caused by a specific line in Sherlock:

        var fillerWords = readConfig("disableRanges") ? patterns.fillerWords2 : patterns.fillerWords;
        ret.eventTitle = ret.eventTitle.split(fillerWords)[0].trim();
        ret.eventTitle = ret.eventTitle.replace(/(?:^| )(?:\.|-$|by$|in$|at$|from$|on$|starts?$|for$|(?:un)?till?$|!|,|;)+/g, '').replace(/ +/g, ' ').trim();
        var match = str.match(new RegExp(helpers.escapeRegExp(ret.eventTitle), "i"));
        if (match) {
          ret.eventTitle = match[0].replace(/ +/g, ' ').trim(); // replace multiple spaces
          if (ret.eventTitle == '')
            ret.eventTitle = null;
        }

To fix this specific bug, all that needs to be done is to set Watson.config["disableRanges"] = true, and this will correctly get parsed as: "Send an email on DLC".

@chrisbutler
Copy link
Member

so i spent a little time trying this out, and while it does work, it actually ends up being this abomination:

global.Watson = ({
	config: { disableRanges: true },
    preprocess: (str) => [str, {}],
    postprocess: (str) => str,
  })

because the sherlock package expects to see Watson as a global (seems like the author isn't really using it in a node/server situation hence the lack of proper module support neilgupta/Sherlock#11)

it also assumes that if the Watson object exists, it should attempt to run both the preprocess and postprocess functions regardless of whether they exist or not (https://github.com/neilgupta/Sherlock/blob/master/sherlock.js#L668)

i kind of want to modify either the Sherlock.parse function to accept that Watson object as a param, or have some sort of Sherlock.init or config function that the object can be passed to (or both, with the former overriding any defaults specified in the latter)...

@dreeves
Copy link
Contributor

dreeves commented May 22, 2018

Related example:
email_habitica_users_re_challenge_todos_within_a_week_of_code_ready got humanized to "email habitica users re challenge todos within of code ready". (link)

Conditional deadlines like that may not be too common (I do like them a lot though) but I'd still prefer the solution of excising all the magic here and humanizing the URL (by which I mean constructing the default title based on the URL) by doing nothing but replacing underscores with spaces and not trying to remove the due date.

@lithp
Copy link
Contributor Author

lithp commented May 23, 2018

@dreeves, I've opened a PR which excises all magic, that's my favorite solution.

@dreeves dreeves added UVI User-Visible Improvement ADO Consensus needed on what to Actually Do (or "much ado about ∅"), AKA question and removed zap (closed as) fixed labels Jun 26, 2018
@dreeves dreeves added this to TRIAGE in Commits.to Jul 13, 2018
@dreeves dreeves moved this from TRIAGE to TODO in Commits.to Jul 16, 2018
@dreeves
Copy link
Contributor

dreeves commented Aug 15, 2018

another example of overclever URL parsing: ..commits.to/create_an_evening_journal_beeminder_goal_by_19_aug_2018 gets a title of “create an journal beeminder goal” because “evening” is a time of day. i advocate completely avoiding these problems by letting the title parse to “create an evening journal beeminder goal by 19 aug 2018”

ie, can we please accept @lithp's PR?

@chrisbutler
Copy link
Member

chrisbutler commented Aug 15, 2018

the PR needed some changes before it was ready to accept as-is (not parsing the title) but the better solution would be to fix sherlock

my es6 branch of sherlock is in pretty good shape: neilgupta/Sherlock@master...commitsto:es6

if we can get a list of all the parsing issues and get some sherlock tests created, we should be able to solve this pretty quickly

then ideally we can keep incrementally improving the parsing of sherlock which improves the app without too much overhead (or duplication)

Commits.to automation moved this from TODO to DONE Aug 17, 2018
@dreeves dreeves added the dup (closed as) duplicate label Aug 19, 2018
@dreeves
Copy link
Contributor

dreeves commented Aug 19, 2018

This is now officially closed as a dup of #164 (which in turn is closed as done, thanks to @lithp's PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADO Consensus needed on what to Actually Do (or "much ado about ∅"), AKA question BUG dup (closed as) duplicate UVI User-Visible Improvement
Projects
Commits.to
  
DONE
Development

No branches or pull requests

3 participants