charmcraft/jujuignore.py: Allow extending the list of patterns. #99
Conversation
charmcraft/commands/build.py
Outdated
| ignore.extend_patterns(ignores) | ||
| return ignore | ||
|
|
||
| def _walk_unignored(self, topdir): |
There was a problem hiding this comment.
Thanks for this.
Anyway, I need to implement the "walk" outside, as it will be ignoring different stuff (a part of which is jujuignored).
So I think this shouldn't be included in this layer.
| def handle_code(self): | ||
| """Handle basic files and the charm source code.""" | ||
| # basic files | ||
| logger.debug("Linking in basic files and charm code") | ||
| self._link_to_buildpath(self.charmdir / CHARM_METADATA) | ||
|
|
There was a problem hiding this comment.
Please reduce this to only one blank line.
charmcraft/jujuignore.py
Outdated
| def _compile_from(self, patterns): | ||
| def extend_patterns(self, patterns: typing.Iterable[str]) -> None: | ||
| """Add more patterns to the ignore list. | ||
| """ |
There was a problem hiding this comment.
Please close the docstring in the same line above.
charmcraft/commands/build.py
Outdated
| @@ -26,6 +26,7 @@ | |||
|
|
|||
| from charmcraft.cmdbase import BaseCommand, CommandError | |||
| from .utils import make_executable | |||
| from .jujuignore import JujuIgnore, default_juju_ignore | |||
There was a problem hiding this comment.
I expect flake8 will fail on these...
There was a problem hiding this comment.
So it was supposed to be "..jujuignore" since it is in the parent directory, but flake8 doesn't complain at that point.
charmcraft/commands/build.py
Outdated
| ignore = JujuIgnore(default_juju_ignore) | ||
| path = self.charmdir / '.jujuignore' | ||
| if path.exists(): | ||
| with path.open() as ignores: |
There was a problem hiding this comment.
Shouldn't we specify the encoding here? Which is the expected encoding for the .jujuignore file?
There was a problem hiding this comment.
.gitignore is officially UTF-8, I don't know that Juju specifically declares an encoding, but there isn't any reason to believe it anything but UTF-8.
| @@ -109,11 +111,20 @@ def _link_to_buildpath(self, srcpath): | |||
| destpath.symlink_to(srcpath) | |||
| return destpath | |||
|
|
|||
| def _load_juju_ignore(self): | |||
There was a problem hiding this comment.
This function needs tests.
| @@ -151,7 +151,11 @@ def __init__(self, patterns: typing.Iterable[str]): | |||
| self._matchers = [] | |||
| self._compile_from(patterns) | |||
|
|
|||
| def _compile_from(self, patterns): | |||
| def extend_patterns(self, patterns: typing.Iterable[str]) -> None: | |||
There was a problem hiding this comment.
Thanks! I'll use this also from tests!
charmcraft/commands/build.py
Outdated
| @@ -26,6 +26,7 @@ | |||
|
|
|||
| from charmcraft.cmdbase import BaseCommand, CommandError | |||
| from .utils import make_executable | |||
| from ..jujuignore import JujuIgnore, default_juju_ignore | |||
There was a problem hiding this comment.
Why not from charmcraft.jujuignore as we do for the rest?
There was a problem hiding this comment.
It's weird. I like from . import foo and from .foo import bar and even from .. import foo, but not from ..foo import bar.
|
I'm happy either way.
…On Wed, Aug 12, 2020, 18:31 Facundo Batista ***@***.***> wrote:
***@***.**** approved this pull request.
Great, thanks!!
------------------------------
In charmcraft/commands/build.py
<#99 (comment)>:
> @@ -26,6 +26,7 @@
from charmcraft.cmdbase import BaseCommand, CommandError
from .utils import make_executable
+from ..jujuignore import JujuIgnore, default_juju_ignore
Why not from charmcraft.jujuignore as we do for the rest?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#99 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7J3UVKAHJEV3CGTOMLSAKRV5ANCNFSM4PLQ3GXQ>
.
|
charmcraft/commands/build.py
Outdated
| @@ -26,6 +26,7 @@ | |||
|
|
|||
| from charmcraft.cmdbase import BaseCommand, CommandError | |||
| from .utils import make_executable | |||
| from ..jujuignore import JujuIgnore, default_juju_ignore | |||
There was a problem hiding this comment.
It's weird. I like from . import foo and from .foo import bar and even from .. import foo, but not from ..foo import bar.
Cleanup a bunch of small issues found in review. (whitespace, bad imports, etc.)
Add tests for _load_jujuignore and its behavior around default ignores and extending those ignores with the content of .jujuignore, as well as clarify the encoding of the file.
jameinel
force-pushed
the
0.3-include-nonignored
branch
from
55cb3b0
to
e803844
Compare
This adds the ability for JujuIgnore to pull in the default ignores and extend them with .jujuignore contents.
I don't have tests around the walk functionality, but I figured I would leave it in there to show you how I was thinking it would look.