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.

Sign up for GitHub

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

Jump to bottom

tweak a couple commands in VS code #477

Merged
merged 7 commits into from
Jun 26, 2019
Merged

tweak a couple commands in VS code #477

merged 7 commits into from
Jun 26, 2019

Conversation

alexboche
Copy link
Contributor

No description provided.

@LexiconCode LexiconCode added the Enhancement Enhancement of an existing feature label May 2, 2019
@alexboche
Copy link
Contributor Author

This is working on my personal branch, but when I tried to tested on a branch equivalent to the latest dictation tools/caster/develop everything worked except for a a couple commands such as 'settings'.I think everything should be okay but would you mind testing this because I think there was something weird going on on my computer?

@LexiconCode LexiconCode added the Testing Required Requesting testers before merge label May 4, 2019
kendonB
kendonB requested changes May 6, 2019
View reviewed changes
Copy link
Collaborator

@kendonB kendonB left a comment

Choose a reason for hiding this comment

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

Looks good - just need to tidy up the rdescripts. Please tidy any others you see as well that don't conform to the style.

castervoice/apps/vscode.py Show resolved Hide resolved
@LexiconCode
Copy link
Member

Take a moment to resolve the conflicts and will see about getting this merged.

@LexiconCode LexiconCode added the Waiting for User Reply Waiting for OP reply label Jun 10, 2019
@alexboche
Copy link
Contributor Author

Take a moment to resolve the conflicts and will see about getting this merged.

It looks like the merge conflicts are basically me adding rdescripts. In a few cases those are unnecessary, but in a few of the merge conflicts I think they are important since the command does not make it obvious what the command does and how it differs from other similar commands. When I resolve the merge conflict, do I need to side with dictation toolbox/caster or can I merge in favor of my changes? In VS code I believe I have the option to accept either my changes or the dictation toolbox/caster changes

@LexiconCode
Copy link
Member

Take a moment to resolve the conflicts and will see about getting this merged.

It looks like the merge conflicts are basically me adding rdescripts. In a few cases those are unnecessary, but in a few of the merge conflicts I think they are important since the command does not make it obvious what the command does and how it differs from other similar commands. When I resolve the merge conflict, do I need to side with dictation toolbox/caster or can I merge in favor of my changes? In VS code I believe I have the option to accept either my changes or the dictation toolbox/caster changes

Merge conflicts exist simply because you your pull request was issued on a version that has had changes. The idea is to respect changes made since by others while integrating your own. In terms of rdescripts feel free adapt that as needed to explain commands in a concise few words or to remove them altogether the command is self-explanatory.

@alexboche
Copy link
Contributor Author

I think this is done,I would remove the "waiting for user reply" tag

@LexiconCode LexiconCode removed the Waiting for User Reply Waiting for OP reply label Jun 22, 2019
@mrob95
Copy link
Member

mrob95 commented Jun 23, 2019

This doesn't work. The non-CCR rule needs to be created first and then referenced in the non property of the CCR rule.

@LexiconCode LexiconCode added Waiting for User Reply Waiting for OP reply Changes Requested A pull request has been reviewed and changes requested and removed Waiting for User Reply Waiting for OP reply labels Jun 23, 2019
@alexboche
Copy link
Contributor Author

alexboche commented Jun 25, 2019 •
edited
Loading

Sorry about that. I just fixed the problem as Mike directed. I also added one command "comment" and moved another "copy path". I tested some command from both rules and they work. I believe this pull request is now done.

@LexiconCode LexiconCode removed the Changes Requested A pull request has been reviewed and changes requested label Jun 25, 2019
@LexiconCode
Copy link
Member

Mike's requested we hold off on changes to applications as he's working in a re-factor. Once that's done this will merge.

@LexiconCode
Copy link
Member

LexiconCode commented Jun 25, 2019 •
edited
Loading

All right @alexboche #604 has been merged. Merged conflicts we should be good to go,

@alexboche
Copy link
Contributor Author

Okay I think this one is done now.

@LexiconCode LexiconCode removed the Testing Required Requesting testers before merge label Jun 26, 2019
@LexiconCode LexiconCode merged commit c72bd12 into dictation-toolbox:develop Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LexiconCode LexiconCode LexiconCode left review comments

@kendonB kendonB kendonB requested changes

Assignees

@alexboche alexboche

Labels
Enhancement Enhancement of an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexboche @LexiconCode @mrob95 @kendonB @Akboche