Skip to content

Conversation

@tewalds
Copy link
Contributor

@tewalds tewalds commented Apr 9, 2025

No description provided.

@mherrmann
Copy link
Owner

Thank you for the PR. Why is it necessary? Can you add a test?

@tewalds
Copy link
Contributor Author

tewalds commented Apr 10, 2025

Thank you, I added a test.

I'm using this in https://github.com/tewalds/replace , which is a simple find/replace tool, ie a worse ripgrep but that actually replaces as well. It's useful to ignore everything that isn't tracked as they're almost certainly generated files or otherwise not interesting, but it kept returning matches/failures from the .git directory, which is also not tracked and not interesting to search. I could add .git to all my .gitignore, but that's per repo and not really correct. An alternative approach would be for gitignore_parser to accept the file string instead of a filename (as the test does), and I could prepend the ".git/" line myself, but this was easier, and I figured you'd appreciate it too.

@mherrmann
Copy link
Owner

I see, thank you for explaining. I understand the need and appreciate the PR, but I feel that a "gitignore parser" should parse .gitignore files, and nothing else. I believe you have a point from a practical perspective, but I'm a purist at heart and can't bring myself to violate that.

@tewalds
Copy link
Contributor Author

tewalds commented Apr 10, 2025

Well, then it'd be nice to have a way of parsing a string instead of a file without mocking open. Would you be open to making that change, or to a PR making that change? That is much more than a couple lines, so I figure you might have some opinions on how to approach it.

@tewalds
Copy link
Contributor Author

tewalds commented Apr 10, 2025

I guess another option would be to make it optional, eg a ignore_git_dir, defaulted to `False.

@mherrmann
Copy link
Owner

Yes, I'm open to a PR that adds a function parse_gitignore_str, so you don't have to mock open.

@tewalds
Copy link
Contributor Author

tewalds commented Apr 10, 2025

Ok, this is a much bigger change, both to the library and the tests, but hopefully you agree this is an improvement. This also fixes #6 along the way.

@tewalds tewalds changed the title Add an implicit ".git/" line which git does internally. Add a parse_gitignore_string so you don't need a real .gitignore file. Apr 10, 2025
@tewalds
Copy link
Contributor Author

tewalds commented Apr 10, 2025

Done.

@tewalds
Copy link
Contributor Author

tewalds commented Apr 11, 2025

The CI seems to be failing due to python versions, so fixed in: #75

tewalds added 5 commits April 11, 2025 11:06
This is useful if you want to list only the files that git would track.
It clearly doesn't track its own files.
…_parse_gitignore_lines, simplify the interface of parse_gitignore_str.
Copy link
Owner

@mherrmann mherrmann left a comment

Choose a reason for hiding this comment

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

If you wanted to add a few lines to the README that explain the new function, that would be great; if not, I can do it as well.

@tewalds
Copy link
Contributor Author

tewalds commented Apr 11, 2025

I added an example to the README as well.

@mherrmann mherrmann merged commit 40c537b into mherrmann:master Apr 14, 2025
5 checks passed
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