Skip to content

Conversation

@cortadocodes
Copy link
Collaborator

@cortadocodes cortadocodes commented Dec 16, 2020

Summary

Prevents imaginary attributes being retrieved from Analysis.

Contents

Minor fixes and improvements

@cortadocodes cortadocodes marked this pull request as ready for review December 16, 2020 17:49
@cortadocodes cortadocodes requested a review from thclark December 16, 2020 17:49
Copy link
Member

@thclark thclark left a comment

Choose a reason for hiding this comment

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

I've just realised why I did this in the first place.
The purpose was to ensure that, once created, users couldn't monkey around with the actual objects, which have a protected UUID attribute.

That way we don't have somebody deciding to override the output_manifest with another Manifest object. Whilst that might not seem a big deal, it'd allow us to keep consistency throughout the process and report what manifest the results will end up in right at the start of the process.

  1. I didn't call super().getattr(self, f"_{name}") anywhere, which is why analysis.furry_purry_catwouldreturn Noneinstead of anAttributeError`

  2. This still needs a test that the output_manifest is created when the twine has an output_manifest strand, I think that hasn't been fixed yet

@cortadocodes
Copy link
Collaborator Author

@thclark would you like the previous behaviour restored and a call to super().getattr(self, f"_{name}") then?

@thclark
Copy link
Member

thclark commented Dec 18, 2020

@thclark would you like the previous behaviour restored and a call to super().getattr(self, f"_{name}") then?

On the one hand we have the philosophy that we should make it as easy as possible for users to do things in sensible ways, but as hard as possible to do things which may have unintended consequences (like pulling the switcharoo on manifests attached to Analysis).

On the other hand, we have our own development efficiency (we're still at an early stage mavericking through this, and if you find it to be easier to debug when the objects are directly attached then we can move through issues quicker) and the general zen of python Simple is better than complex. - may well just lead to fewer bugs exposed to users (like this one!) and less code for us to maintain.

I perhaps lean toward the latter but am torn (and am also suffering from the sunk cost fallacy, because I wrote the code in the first place). Feel free to decide the approach you favour, I'll support you 💯

@cortadocodes
Copy link
Collaborator Author

I'm in favour of the latter too as I think the extra complexity may lead to inadvertent bugs. I'm also thinking that most users won't need to touch the analysis object or anything else if they're using the CLI - is this a correct assumption? Even if they do need/want to, I'm think that the user should be trusted not to mess around with the attributes of objects that have been produced for them via a black box, and if they do change them, it would be reasonable to expect there might be a problem

Copy link
Member

@thclark thclark left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines -112 to +113
# Otherwise do nothing and rely on manifest having its path variable set already
return manifest
# Otherwise do nothing and rely on manifest having its path variable set already
return manifest
Copy link
Member

Choose a reason for hiding this comment

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

Aha!!

@cortadocodes cortadocodes changed the title Prevent imaginary attributes being retrieved from Analysis Prevent imaginary/empty attributes being retrieved from Analysis Dec 18, 2020
@cortadocodes cortadocodes merged commit 4879ee3 into release/0.1.6 Dec 18, 2020
@cortadocodes cortadocodes deleted the fix/empty-analysis-attributes branch December 18, 2020 12:46
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.

3 participants