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

bug fixes and cleanup. #16

Merged
merged 11 commits into from
May 13, 2014
Merged

bug fixes and cleanup. #16

merged 11 commits into from
May 13, 2014

Conversation

adam-zethraeus
Copy link
Contributor

I want to put this up for PR despite having collab status because it makes some stylistic changes, and i don't want to step on your toes. :)
(the changes are a bit less deep than the line count implies, there's a bit of reordering and renaming)

it fixes:

  • zero word truncation bug
  • trailing tags in char mode bug

notable changes:

  • i've tried to make the state more explicit (grouped into trackedState and options objs)
  • introduces more explicit mode for chars vs words
  • i've separated limit checking and counting into two different functions
  • limit checking now happens first (which is what fixes trailing tags in character mode. #14 and allows for removal of the 'characters + 1' thing)

i hope it's alright. lmk.

* added braces to single line conditionals
* removed redundant argument to count() when already in closure
* tweaked some comments
* removed characters + 1 hack
* switched order of limit check and character addition
* introduced one bit of weirdness requiring spaces after final word to be trimmed - but this should be solvable and i judge it worth it for the fixes of above edge cases in functionality and behaviour!
@adam-zethraeus adam-zethraeus changed the title zero bug fix and cleanup bug fixes and cleanup. Apr 28, 2014
@cgiffard
Copy link
Owner

Thanks — whoah that's a heck of a PR. :) I'll have a look when I get to work.

@cgiffard
Copy link
Owner

So far, I really like this! Thanks for keeping me in the loop by filing this as a PR rather than just pushing to master.

I haven't had an opportunity to do a proper review, but I'll try get it done ASAP so we can get this in. :)

Thanks again!

@adam-zethraeus
Copy link
Contributor Author

Could you give an ETA on this / maybe just a general assurance that it's ok for me to develop on top of these changes?

@adam-zethraeus
Copy link
Contributor Author

ping?

@cgiffard
Copy link
Owner

Sorry, this one slipped. I figure you can merge it — it looks good! Let me know when you've done it, I'll push an update to npm.

FYI the general direction I want to take this is:

  • Anything can be theoretically counted — so we need to maintain an arbitrary 'count' function.
  • We want to maintain a proper state machine for parsing, with no lookahead if possible. No regex or anything should be ever used in parsing input, except for detecting character ranges.
  • I want this to eventually work with streaming input. (hence no lookahead.)

Does that sound reasonable — have I missed anything? :)

adam-zethraeus added a commit that referenced this pull request May 13, 2014
@adam-zethraeus adam-zethraeus merged commit e65fe59 into master May 13, 2014
@adam-zethraeus
Copy link
Contributor Author

all in, ready to npm.

direction makes sense! I have no idea if the streaming input with no lookahead is viable. Does that necessitate a char being released for every one coming in?

I don't know when I'll do it, but my next push will be to try to build some sort of special case handling in for the ghost image thing, but as generically as possible. I'll look at making count generic and implementing block counting after that.

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

Successfully merging this pull request may close these issues.

trailing tags in character mode.
2 participants