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

Clean up #81

Closed
wants to merge 38 commits into from
Closed

Clean up #81

wants to merge 38 commits into from

Conversation

boyter
Copy link
Owner

@boyter boyter commented Jun 17, 2019

Not quite ready for review, mostly here so I can see what im doing more easily, oh and so I get travis to do some builds so I can see if I broke things.

@boyter boyter requested a review from dbaggerman June 17, 2019 22:45
@dbaggerman
Copy link
Collaborator

Thanks for the push notification spam!

Joking aside, I'm not a fan of the sta variable name. I think I'd go with making the struct name more explicit (e.g. scannerState), and calling the variable state (or scanner?).

If we're trimming down to a single argument everywhere, there's also the option of using method syntax. state.stringState() might be a bit opaque, but something like scanner.scanString() doesn't seem too bad.

@boyter
Copy link
Owner Author

boyter commented Jun 18, 2019

Hahaha yeah I forgot it would email you a lot. Sorry about that.

Yep I don't like the name either. Once I finish this off ill change it.

@boyter boyter closed this Oct 10, 2019
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

2 participants