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

feat: enable interaction after record args validation failure, option to #89

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

pbheemag
Copy link
Contributor

append quit to replay file, update messaging, continue session after recording

Issue #, if available:

Description of changes:

  • enable interaction after record args validation failure.
  • option to append .quit to replay file.
  • Update messaging when command is executed outside cli project.
  • session persistence once record is done.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pbheemag pbheemag force-pushed the dialog_command_bug_fix_feature_update branch from 0dc79de to 0c1d7cb Compare March 31, 2020 04:43
Copy link
Contributor

@Chih-Ying Chih-Ying left a comment

Choose a reason for hiding this comment

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

The changes looks good, but I found two issues:

  1. the first one is a following issue, in git-credentials-helper getSkillId via ResourcesConfig is undefined (https://github.com/alexa-labs/ask-cli/blob/fix_ahs_skillid_not_in_state/lib/commands/util/git-credentials-helper/index.js#L40), so get git-credentials is always failed. I looked into a upgraded skill, skillId only exists in ask-resources.json, but ResourcesConfig get skillId from ask-states.json (https://github.com/alexa-labs/ask-cli/blob/fix_ahs_skillid_not_in_state/lib/model/resources-config/index.js#L66), should we change it back to ask-resources.json?
  2. the second issue is not related to this PR, we should add ask-resources.json into .gitignore while upgrading project, I can create another PR to fix it?

@Chih-Ying Chih-Ying dismissed their stale review March 31, 2020 05:21

Put comments in wrong PR

lib/commands/dialog/index.js Outdated Show resolved Hide resolved
lib/commands/dialog/index.js Outdated Show resolved Hide resolved
lib/controllers/dialog-controller/index.js Outdated Show resolved Hide resolved
append quit to replay file, update messaging, continue session after recording
@pbheemag pbheemag force-pushed the dialog_command_bug_fix_feature_update branch from 0c1d7cb to fb25d10 Compare March 31, 2020 07:21
@RonWang
Copy link
Contributor

RonWang commented Mar 31, 2020

Btw this one is not a feat, so I will check it in as a fix once Travis CI passes.

@RonWang RonWang merged commit ea8da7a into develop Mar 31, 2020
@RonWang RonWang deleted the dialog_command_bug_fix_feature_update branch March 31, 2020 07:27
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.

4 participants