Skip to content

Conversation

@c3sxr
Copy link

@c3sxr c3sxr commented Oct 2, 2020

If the digit is D2 (@ special character) then it will go back one character and the next will replace it
I included a condition to avoid this and include the @ special character as an input character

Copy link
Owner

@afucher afucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR!

I added some suggestion about the fix, and also it will be good to add a test to replicate this error. If you need any help with this, please let me know.

…ed validation on ConsoleObservable:ImplementKeysBehaviours and in ConsoleObservable:GetLineObservable to allow @ character
@afucher
Copy link
Owner

afucher commented Oct 14, 2020

hey @c3sxr , could you please add some tests for this bug?

@c3sxr
Copy link
Author

c3sxr commented Oct 14, 2020

Hi @afucher !
I made another push to the forked branch, I included a new class for the email test, applied the changes and returned the version back.
Let me know if there's something missing, I'll be glad to help 👍
Thank you!

@afucher
Copy link
Owner

afucher commented Oct 14, 2020

Hey @c3sxr for some reason it is not reflecting any new changes here in the Pull Request 😕

@c3sxr c3sxr requested a review from afucher October 15, 2020 00:23
@c3sxr
Copy link
Author

c3sxr commented Oct 15, 2020

Can you please look again, the File changes tab is showing my new changes
image
Thanks!

@afucher
Copy link
Owner

afucher commented Oct 15, 2020

Oh those files I saw, my comment was regarding adding a new test that cover the @ bug

@c3sxr
Copy link
Author

c3sxr commented Oct 16, 2020

Oh you mean on the InquirerUnitTest? Sure let me include it

@c3sxr
Copy link
Author

c3sxr commented Oct 16, 2020

Can you please take a look and let me know if that looks right?

case 'ü':
return new ConsoleKeyInfo('ü', ConsoleKey.U, false, false, false);
case '@':
return new ConsoleKeyInfo('@', ConsoleKey.D2, false, false, false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the first boolean parameter represent the shift key, and in this case it should be true. Could you check please?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right the parameter shift needed to be true. Can you please review again?
Thanks!

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.

3 participants