cli: setup gitignore when working from git directory at init. #1580

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants

msis commented Sep 29, 2017

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

This adds predefined .gitignore if .git directory exists.

kyrofa approved these changes Oct 2, 2017 edited

Thanks @msis! I've tested this successfully and I like it, although I suggest a few code tweaks before I can wholeheartedly give it a +1.

snapcraft/internal/lifecycle.py
@@ -88,6 +97,15 @@ def init():
with open(snapcraft_yaml_path, mode='w') as f:
f.write(yaml)
+ git_cache = os.path.join('.git')
@kyrofa

kyrofa Oct 2, 2017

Member

The use of os.path.join is unnecessary, here. In fact, I think this entire variable is unnecessary, no?

snapcraft/internal/lifecycle.py
+ if os.path.exists(git_cache):
+ if os.path.exists('.gitignore'):
+ pass
+ else:
@kyrofa

kyrofa Oct 2, 2017

Member

Rather than passing, perhaps this would be easier to read:

# Don't mess with an existing .gitignore
if not os.path.exists('.gitignore'):
    with open('.gitignore', mode='w') as f:
        f.write(_TEMPLATE_GITIGNORE)
Member

kyrofa commented Oct 2, 2017

Because I apparently don't know how to use the review tools, I gave it a wholehearted +1 anyway. That's okay 😄 .

msis commented Oct 3, 2017

Thanks @kyrofa !
I updated the code to follow your guidelines.
PS: Do I need to register somewhere for that integration test to succeed?

Member

kyrofa commented Oct 3, 2017

@msis no, you're good. Something else is broken in that check, so I invoke @elopio.

Neat :-D Love at first sight!

Collaborator

sergiusens commented Oct 5, 2017

Looks good, mind if I move stuff around in the PR a bit? Just use of dedent and driving the ignore creation from the cli

snapcraft/tests/commands/test_init.py
+ def test_init_must_not_write_gitignore_if_no_git_dir(self):
+ self.run_command(['init'])
+
+ # Verify the .gitignore was created
@elopio

elopio Oct 7, 2017

Member

There's a typo here: Verify the .gitignore was NOT created.
I would just remove this comment, the assertion is clear enough, I think.

This is great, thanks @msis!
There is one typo in one of the comments, so I'll use this opportunity to be nitpicky and the three # Verify ... comments. It seems to me that the name of the test + the nice assertions you have there are very clear and readable.

Also, as Sergio mentioned, please use dedent. It's a very recent rule in our code style book: https://github.com/snapcore/snapcraft/blob/master/CODE_STYLE.md#multiline-strings

Member

elopio commented Oct 24, 2017

Sorry @msis, I delayed my review and now this pull request has a conflict. Can you please fix it?

@sergiusens sergiusens added this to the 2.36 milestone Oct 26, 2017

Collaborator

sergiusens commented Nov 28, 2017

thanks for the contribution. I think we will need some more smarts in this to make it bullet proof and remove any potential error prone behavior.

@sergiusens sergiusens closed this Nov 28, 2017

@sergiusens sergiusens removed this from the 2.36 milestone Nov 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment