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

Sniff all strings to check if they're valid mathematical expressions #710

Closed

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 20, 2012

This now sniffs
=5_5
5_5
555

and correctly sets them to be formula types

It means there's no obligation to type '=', but you still can to go into text mode.
It also won't assign a QSFormula type unless you have the Calculator Module installed.

If anybody has any ideas on the warning with performAction then that'd be appreciated. Not sure how to get rid of it.

This now sniffs
=5*5
5*5
555

and correctly sets them to be formula types
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 20, 2012

Oh, P.S.:

It also returns if the string has no length. Previously QS would run through the whole sniffString method when you:

  • enter a character in text mode
  • delete the character so you have an empty text area

@skurfer
Copy link
Member

@skurfer skurfer commented Feb 22, 2012

Two questions:

  1. Does this require the changes to the Calculator plug-in to work correctly? If I type a number in text mode, it shows up twice and the text panel won’t go away until I relaunch.
  2. It looks like it runs the performCalculation: action and compares the result every time you hit a key. Is that right? Is that what we want?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 22, 2012

Does this require the changes to the Calculator plug-in to work correctly?

Yes.

It looks like it runs the performCalculation: action and compares the result every time you hit a key. Is that right? Is that what we want?

Yes. Now I think about it, it does seem like it's a little wasteful, but I think it's the best way to check if a string is a mathematical expression. .eg. how else would you check "sin(55) + log(5^2)" ?

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Feb 23, 2012

I have a style related observation: the sniffString method is getting quite long. Maybe you could split it up into smaller methods (you know, checkFormula, checkFilePath, checkUrl or things like that) and call each one from the sniffString method. I think it would make the whole thing more easy to read.

Now I think about it, it does seem like it's a little wasteful

You could run some tests to check which of the parts of sniffString are quickest and which take the longest. Then you could move them around, so the quickest check are run first and if they are successful, you wont have to run the other ones. E.g. a string wont be a mathematical expression if we have already determined its an email address.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Feb 23, 2012

Good points on both accounts. I will look into it and update the pull
request (when I get time)

QL integration is now my priority (as I've emailed Lifehacker)

On 23 February 2012 09:50, Henning Jungkurth <
reply@reply.github.com

wrote:

I have a style related observation: the sniffString method is getting
quite long. Maybe you could split it up into smaller methods (you know,
checkFormula, checkFilePath, checkUrl or things like that) and call
each one from the sniffString method. I think it would make the whole
thing more easy to read.

Now I think about it, it does seem like it's a little wasteful

You could run some tests to check which of the parts of sniffString are
quickest and which take the longest. Then you could move them around, so
the quickest check are run first and if they are successful, you wont have
to run the other ones. E.g. a string wont be a mathematical expression if
we have already determined its an email address.


Reply to this email directly or view it on GitHub:
#710 (comment)

@skurfer
Copy link
Member

@skurfer skurfer commented Feb 23, 2012

how else would you check "sin(55) + log(5^2)"

I probably wouldn’t. If you’re willing to type all that, I don’t think adding and c is too much to ask (or alternately typing = up front). ;-) In other words, I don’t see a problem that needs to be solved here. You obviously do or you wouldn’t have taken the time to implement this, so I don’t want to dismiss the idea. But I still need some convincing.

My main concern is what we’ve already been talking about: This method runs with every keystroke, so it needs to be lightweight. I think the method was in good shape as far as doing the quicker checks first, but if we’re going to make this change, the formula checks should probably come later.

I have no strong feelings either way about breaking sniffString into smaller pieces. It’s long, but with proper comments, I think it’s easy enough to follow.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 12, 2012

Closing this. Hopefully with Rob's idea of sniffString augmentation we can implement something like this/makie it better

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

Successfully merging this pull request may close these issues.

None yet

3 participants