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

Adding support for avaya_ers_show_logging_config command #301

Merged

Conversation

kadecole
Copy link
Contributor

ISSUE TYPE
  • New Template Pull Request
COMPONENT

avaya_ers_show_logging_config

SUMMARY

Adding new template for Avaya ERS for show logging config command


Copy link
Contributor

@jmcgill298 jmcgill298 left a comment

Choose a reason for hiding this comment

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

It seems that you are just capturing the text after :, so this can be simplified to just use (.+?) for the capture group and mark the end of the line \s*$$

New templates require raising errors for raw lines that are not parsed so we can ensure we know we have parsed the data correctly; my suggestions have also included this.

I am also not sure what Done is doing at the end of the template file; can you please inform us how that is being used?

templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
templates/avaya_ers_show_logging_config.template Outdated Show resolved Hide resolved
@kadecole
Copy link
Contributor Author

Thanks for the code review. I have updated the PR with some of the changes requested.

I based this template off previous templates and they showed Done on the last line of the file. If this Done is not needed I will remove.

Also for the other code review suggestions, I am matching on what should be returned. There should only be two options for the text returning in those fields.

@jmcgill298
Copy link
Contributor

I still think it is safer to use (.+?) in case they change caps, short forms, or add options in the future, but I am okay to leave it like it is. However, I would like to remove Done since it should not be doing anything, and I think it is confusing for anyone looking at this later. Once that is done, I am good to merge.

@kadecole
Copy link
Contributor Author

I have updated from your suggestions. Please review and merge when possible. Thanks.

@jmcgill298 jmcgill298 merged commit 7121ac1 into networktocode:master Nov 21, 2018
@kadecole kadecole deleted the avaya-ers-show-logging-config branch November 21, 2018 15:49
jvanderaa pushed a commit that referenced this pull request Nov 10, 2021
* Initial Commit

* Apply suggestions from code review

Co-Authored-By: kadecole <[email protected]>

* Updates from code review
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.

2 participants