Writeme: integrate folder exclusion into writeme and fix validation issues #7539
Conversation
github-actions
bot
added
Python
beqqrry-aws
requested review from
a team and
rlhagerm
and removed request for
a team
beqqrry-aws
added
the
Task
- Add folder exclusion functionality to runner.py as a core feature - Support excluding multiple configurable folders (.kiro, .git, node_modules, __pycache__) - Add proper exit code handling to writeme.py wrapper - Fix malformed snippet-end tag in BatchActions.java - Update generated README files for KMS and Lambda services - Ensure proper validation error reporting
- Remove 'patch' references from function names and comments - Rename apply_folder_exclusion_patches() to _configure_folder_exclusion() - Rename patched_skip() to enhanced_skip() - Rename patched_get_files() to enhanced_get_files() - Update comments to reflect natural functionality rather than retrofitted patches
beqqrry-aws
force-pushed
the
writeme-validation-fixes
branch
from
ef3baa2 to
e32c8db
Compare
| )) | ||
|
|
||
| except Exception as e: | ||
| # Skip files that can't be read (binary files, etc.) |
There was a problem hiding this comment.
All exceptions are files that can't be read? What if the file is missing?
There was a problem hiding this comment.
how would this know if a file is missing? It just wouldn't show up in the list to check then, right?
There was a problem hiding this comment.
Or, misnamed, maybe. I was questioning the very wide net cast by that except, and the assumption that all the errors are of one type.
| @@ -24,7 +24,14 @@ | |||
|
|
|||
| from typer import run | |||
|
|
|||
| run(writeme) | |||
| # Run writeme and ensure proper exit code handling | |||
| try: | |||
There was a problem hiding this comment.
I thought we added exit code handing by using typer.exit in runner.py? Is this necessary here also?
There was a problem hiding this comment.
should writeme.py assume runner.py is handling all possibilities?
|
|
||
| # Check that every snippet-start has a corresponding snippet-end | ||
| for tag, start_line in snippet_starts.items(): | ||
| if tag not in snippet_ends: |
There was a problem hiding this comment.
This code that checks for matching starts and ends, doesn't it duplicate what's in -tools in snippets.py. Is there a problem with that logic or a reason we should duplicate it here?
There was a problem hiding this comment.
There is at least some problem with the previous logic, because this caught the error in BatchActions.java which boils down to a missing end tag (malformed)
There was a problem hiding this comment.
In that case, could you upstream a fix for that?
There was a problem hiding this comment.
Oh, not a missing tag, but a malformed one? Yeah, I bet the existing logic (and related tests) should be modified to handle that.
| root: Path, skip: Callable[[Path], bool] = lambda _: False, fs: Fs = PathFs() | ||
| ) -> Generator[Path, None, None]: | ||
| """Get files using enhanced skip function.""" | ||
| for path in file_utils.walk_with_gitignore(root, fs=fs): |
There was a problem hiding this comment.
if walk_with_git_ignore is ignoring everything in the git ignore, can we just add these directories to that?
There was a problem hiding this comment.
I don't think they're guaranteed to be 1:1, but it's probably pretty close
There was a problem hiding this comment.
Ok, then, this seems like duplicated functionality.
There was a problem hiding this comment.
walk_with_gitignore should work at any level of gitignore granularity.
{'.kiro', '.git', 'node_modules', '__pycache__'}
These should definitely be in your .gitignore already? I suppose I can see an argument for .kiro
return path.suffix.lower() not in validator_config.EXT_LOOKUP or path.name in validator_config.IGNORE_FILES
Again, seems like *.ext in the .gitignore or path in the folders' .gitignore would be equivalent?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.