New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python plugin: record manifest #1487

Merged
merged 9 commits into from Aug 22, 2017

Conversation

Projects
None yet
4 participants
@elopio
Member

elopio commented Aug 14, 2017

No description provided.

@sergiusens

This comment has been minimized.

Show comment
Hide comment
@sergiusens

sergiusens Aug 17, 2017

Collaborator

It feels like storing the requirements file is almost informational now, isn't it?

Given that we record the exact packages to install and can potentially use constraints to avoid some weird behaviour (unbeknownst to us) of pip picking packages outside of what was locked down. I guess the reason we store requirements files is in case it is something external to the sources of the project.

I don't have any critique here, just thinking out loud as this has prevented me from reviewing properly since I first saw the PR Monday night.

Collaborator

sergiusens commented Aug 17, 2017

It feels like storing the requirements file is almost informational now, isn't it?

Given that we record the exact packages to install and can potentially use constraints to avoid some weird behaviour (unbeknownst to us) of pip picking packages outside of what was locked down. I guess the reason we store requirements files is in case it is something external to the sources of the project.

I don't have any critique here, just thinking out loud as this has prevented me from reviewing properly since I first saw the PR Monday night.

@sergiusens

This comment has been minimized.

Show comment
Hide comment
@sergiusens

sergiusens Aug 17, 2017

Collaborator

Since testing was brought up, now that we are using testtools, I much prefer

self.assertThat(output, Equals(expected)

instead of

self.assertEqual(output, expected)

since in the latter when writing (made easier with good variable naming) or executing it is never clear what is what you wanted versus what you got.

Collaborator

sergiusens commented Aug 17, 2017

Since testing was brought up, now that we are using testtools, I much prefer

self.assertThat(output, Equals(expected)

instead of

self.assertEqual(output, expected)

since in the latter when writing (made easier with good variable naming) or executing it is never clear what is what you wanted versus what you got.

@kyrofa

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Aug 17, 2017

Member

I much prefer

self.assertThat(output, Equals(expected)

instead of

self.assertEqual(output, expected)

since in the latter when writing (made easier with good variable naming) or executing it is never clear what is what you wanted versus what you got.

Agreed, especially when you consider that, with testtools, it's assertEqual(expected, actual) where order matters. All of the tests we wrote before the switch had it the other way around, since order didn't matter.

Member

kyrofa commented Aug 17, 2017

I much prefer

self.assertThat(output, Equals(expected)

instead of

self.assertEqual(output, expected)

since in the latter when writing (made easier with good variable naming) or executing it is never clear what is what you wanted versus what you got.

Agreed, especially when you consider that, with testtools, it's assertEqual(expected, actual) where order matters. All of the tests we wrote before the switch had it the other way around, since order didn't matter.

@elopio

This comment has been minimized.

Show comment
Hide comment
@elopio

elopio Aug 18, 2017

Member

Alright, thanks for the reviews!
I updated the decorator, assertThat, and modified the code to only record the file contents if they are remote.
Please take another look.

Member

elopio commented Aug 18, 2017

Alright, thanks for the reviews!
I updated the decorator, assertThat, and modified the code to only record the file contents if they are remote.
Please take another look.

@kyrofa

kyrofa approved these changes Aug 22, 2017

Looks good to me now.

@sergiusens

Looks good, but please improve the check for locality of requirements txt in new PR, this should be implemented in sources to verify a file is actually committed into the VCS

installed_pipy_packages = self._run_pip(setup_file)
# We record the requirements and constraints files only if they are
# remote. If they are local, they are already tracked with the source.
if self.options.requirements and isurl(self.options.requirements):

This comment has been minimized.

@sergiusens

sergiusens Aug 22, 2017

Collaborator

this check need improvement as != url does not imply it is part of the source. You can do this in a follow up PR though.

@sergiusens

sergiusens Aug 22, 2017

Collaborator

this check need improvement as != url does not imply it is part of the source. You can do this in a follow up PR though.

@sergiusens

This comment has been minimized.

Show comment
Hide comment
@sergiusens

sergiusens Aug 22, 2017

Collaborator

Fixes #1454

Collaborator

sergiusens commented Aug 22, 2017

Fixes #1454

@sergiusens sergiusens merged commit 5887a82 into snapcore:master Aug 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

kalikiana added a commit to kalikiana/snapcraft that referenced this pull request Sep 21, 2017

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