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

Handle yamllint_config issues with error messages #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bjhargrave
Copy link
Contributor

If the specified yamllint_config is empty or malformed, we want to supply an appropriate error message.

Closes #60

If the specified yamllint_config is empty or malformed, we want to
supply an appropriate error message.

Signed-off-by: BJ Hargrave <[email protected]>
@reidliu41
Copy link

seems it will show only failed msg if invalid, may no need to go next:

        lines = result.stdout.splitlines()
        logger.debug(f"====================xx=================> lines: {lines}")
        
        DEBUG 2024-10-24 09:38:13,551 instructlab.schema.taxonomy:286: ====================xx=================> lines: ['invalid config: not a dict']

@bjhargrave
Copy link
Contributor Author

seems it will show only failed msg if invalid, may no need to go next:

I don't understand your comment. Can you be more specific? Thanks

@reidliu41
Copy link

sorry, not that clear. It should only show the invalid if failed, so may no need to check it in the next step, like pattern, the for-loop.

@bjhargrave
Copy link
Contributor Author

It should only show the invalid if failed, so may no need to check it in the next step, like pattern, the for-loop.

In the loop we have established that the list exists and is non-empty. The output will either be empty (no messages because everything was fine, so the loop will not run), a list of messages (which will match the pattern, so the elifs will be ignored), or some other message such as invalid config or a traceback due to malformed config (in this case we are in "error" mode so no need for performance concerns and the elifs return so the loop is terminated). So I think the changes in this PR are correct.

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