Add pattern checks for linting #1374
Conversation
nimbinatus
force-pushed
the
taxonomy-1369
branch
from
095906e to
eb03f4e
Compare
Signed-off-by: Laura Santamaria <[email protected]>
…tructlab#1369 Signed-off-by: Laura Santamaria <[email protected]>
Signed-off-by: Laura Santamaria <[email protected]>
nimbinatus
force-pushed
the
taxonomy-1369
branch
from
eb03f4e to
9a7d72a
Compare
|
(Also, does squash and merge not work here in this repo, so I need to squash and force push to clean up history before merging on my own?) |
| if taxonomy.errors > 0: | ||
| exit_code = 1 | ||
| if taxonomy.warnings > 0: |
There was a problem hiding this comment.
This is wrong since N errors and 1 warning means exit code 0. These 2 lines are not needed since exit_code is initialized to 0.
There was a problem hiding this comment.
Oh bugger, you're right; I'm resetting the exit code. Sorry; fixing that.
| # maintainers to address rather than block on them. We will | ||
| # revisit when other content is allowed. | ||
| qna_file_path = taxonomy.rel_path.with_name("qna.yaml") | ||
| if "knowledge" in qna_file_path.parts: |
There was a problem hiding this comment.
This may be too promiscuous since the user could put "knowledge" some where in their sub path: compositional_skills/philosophy/knowledge/qna.yaml
I think you need to look at only part 0.
| # revisit when other content is allowed. | ||
| qna_file_path = taxonomy.rel_path.with_name("qna.yaml") | ||
| if "knowledge" in qna_file_path.parts: | ||
| qna_file_contents = parser.parse(qna_file_path).contents |
There was a problem hiding this comment.
The qna.yaml is already parsed in the taxonomy object: taxonomy.contents. Why parse it again?
| if "knowledge" in qna_file_path.parts: | ||
| qna_file_contents = parser.parse(qna_file_path).contents | ||
| for element in qna_file_contents["document"]["patterns"]: | ||
| if not re.match('.*.md', element): |
There was a problem hiding this comment.
I thought pdf support was added? Anyway the regex here would be
re.search("\.md$", element)
But what about the pattern folder_of_md_files/* which is a legitimate value which should not be rejected.
If you want to do more checking here, I don't think you can do it by pattern matching the yaml contents. You would need to clone the repo, find all files in the repo which match the patterns, and then check that all those files match the desired file types.
| qna_file_contents["document"]["repo"]): | ||
| taxonomy.warning( | ||
| "The document repo \"%s\" needs to be a " | ||
| "GitHub-based repository.", |
There was a problem hiding this comment.
No it doesn't have to be GitHub. Any valid git repo could be used. We just expect that any such git repo can be accessed because any necessary authorization is configured.
| "GitHub-based repository.", | ||
| qna_file_contents["document"]["repo"] | ||
| ) | ||
| if not re.match( |
There was a problem hiding this comment.
This potential check is discussed here: instructlab/schema#30
If we do want to require SHA values, we should probably do that in the schema. But I am not convinced that is a great idea. Since we could allow non-SHA values that have special meanings.
Fixes #1369
Adds in three pattern checks to address issues discovered during triage. Warns as these will not break code, but rather are specific to legal requirements (best actual documentation I can find is https://github.com/instructlab/taxonomy/blob/main/README.md?plain=1#L284-L287, but this information was discussed in a triage meeting with @jjasghar and @juliadenham).
Also, hides
uv-specific files from git.