Skip to content

[refactor] Use Path instead of os #19

Closed
jurgenwigg wants to merge 2 commits intonocomplexity:mainfrom
jurgenwigg:feat/move-to-path
Closed

[refactor] Use Path instead of os #19
jurgenwigg wants to merge 2 commits intonocomplexity:mainfrom
jurgenwigg:feat/move-to-path

Conversation

@jurgenwigg
Copy link
Copy Markdown
Contributor

  • Change os to Path use.
  • Refactor of the collect_python_source_files function to be simpler

@jurgenwigg jurgenwigg marked this pull request as ready for review April 17, 2026 17:20
@nocomplexity
Copy link
Copy Markdown
Owner

Thank you again for taking the time to submit this PR. It’s much appreciated!

Could you please remove the .gitignore from this branch? I try to keep commits atomic (focused on one specific task), and keeping the environment hygiene separate from functional code helps avoid friction during merges.

On the technical side, switching to pathlib.Path instead of os is a great shout—it definitely makes the read_in_source_file function more robust.

I need to review the logic in depth and potentially write a few more test scripts. Since this function is used for almost all the core functionality, the current test coverage isn't quite high enough to merge this blindly.

I’ll aim to get this merged (without the .gitignore change) this coming week. Unfortunately, I won't be able to get to it this weekend due to other work priorities.

Regards!

@jurgenwigg
Copy link
Copy Markdown
Contributor Author

jurgenwigg commented Apr 17, 2026

Ok, I'll remove that file :) if you prefer, I can write tests for that function.

[Update]
File removed.

@nocomplexity
Copy link
Copy Markdown
Owner

Thanks again for this great work!

It really inspired me to perform an in-depth review of the current code and evaluate the merge. Unfortunately, I have decided not to merge this PR, as the proposed changes to the collect_python_source_files function would introduce subtle side effects.

Your proposed version has full path traversal, which should be avoided.
A Side effect of full path traversal is different I/O behaviour and possible permission errors for . directories that no are excluded by default on purpose.

rglob can be case-sensitive depending on the OS. I use now endswith(".py"). This is now case-sensitive on purpose.

I am also concerned that these changes might break current logic on Windows and for the WASM version, especially as the testing on Windows is not yet optimally organised.

Your PR inspired me to add some extra unit tests so improvements to this function can be made when needed in the future.

Regards!

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