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

Fixed first character removal #6

Closed
wants to merge 1 commit into from
Closed

Fixed first character removal #6

wants to merge 1 commit into from

Conversation

bezineb5
Copy link
Contributor

@bezineb5 bezineb5 commented Jun 2, 2017

The library was crashing when the sentence contained multiple $ characters, such as:
$GPTXT,01,01,02,u-blo?$GPTXT,01,01,02,u-b$GPTXT,01,01,02,u-bl?$GPTXT,01,01,02,u-blox ag - www.u-blox.co?$GPTXT,01,01,02,u-blox ag - www.u-blox.com*50
(I agree it's invalid, but that makes the library crash)

The library was crashing when the sentence contained multiple $ characters, such as:
$GPTXT,01,01,02,u-blo?$GPTXT,01,01,02,u-b$GPTXT,01,01,02,u-bl?$GPTXT,01,01,02,u-blox ag - www.u-blox.co?$GPTXT,01,01,02,u-blox ag - www.u-blox.com*50
(I agree it's invalid, but that makes the library crash)
@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage remained the same at 86.588% when pulling 57db201 on bezineb5:fix-trim-$ into 523439f on adrianmo:master.

@adrianmo
Copy link
Owner

adrianmo commented Jun 2, 2017

Hi @bezineb5,

I'm not sure what is the problem you are trying to fix. The example you provided doesn't look good and the sentence type (ie. GPTXT) is not even supported by this lib yet. Can you please clarify your use case?

That said, I think you brought a good point (maybe unintentionally?). If there is more than one $ in the sentence, we should probably only consider the last one and ignore the ones in the left.

Check out this example: https://play.golang.org/p/1beUJTWMvO

I've added a few sentences and tested the current solution, your proposed solution, and a new one that might work in the scenario I mentioned.

Let me know what you think.

@adrianmo adrianmo self-assigned this Jun 2, 2017
@bezineb5
Copy link
Contributor Author

bezineb5 commented Jun 2, 2017

I'm reworking it with a difference pull request to take your comments into account.

@bezineb5 bezineb5 closed this Jun 2, 2017
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