Skip to content

Conversation

@lasley
Copy link
Member

@lasley lasley commented Dec 16, 2016

This is a WIP refactoring and is also dependent on:

@lasley lasley added this to the 0.1.0 milestone Dec 16, 2016
@lasley lasley self-assigned this Dec 16, 2016
@lasley lasley force-pushed the feature/master/certificate-request branch 11 times, most recently from 701a944 to 3753138 Compare December 16, 2016 22:08
@codecov-io
Copy link

codecov-io commented Dec 16, 2016

Current coverage is 100% (diff: 100%)

Merging #4 into master will not change coverage

@@           master    #4   diff @@
===================================
  Files           2    13    +11   
  Lines          48   130    +82   
  Methods         0     0          
  Messages        0     0          
  Branches        2     5     +3   
===================================
+ Hits           48   130    +82   
  Misses          0     0          
  Partials        0     0          

Sunburst

Diff Coverage File Path
•••••••••• 100% new cfssl/models/config_key.py
•••••••••• 100% new cfssl/models/certificate_request.py
•••••••••• 100% new cfssl/models/policy_auth.py
•••••••••• 100% new cfssl/models/policy_sign.py
•••••••••• 100% new cfssl/defaults.py
•••••••••• 100% new cfssl/models/config_client.py
•••••••••• 100% new cfssl/models/policy_use.py
•••••••••• 100% cfssl/cfssl.py
•••••••••• 100% new cfssl/models/host.py
•••••••••• 100% new cfssl/models/config_mixer.py

Review all 12 files changed

Powered by Codecov. Last update 5418e0c...92471a6

@lasley lasley force-pushed the feature/master/certificate-request branch 10 times, most recently from 18c1c0b to d5f1823 Compare December 17, 2016 02:46
@lasley lasley requested a review from tedsalmon December 19, 2016 23:16
@lasley lasley force-pushed the feature/master/certificate-request branch from d5f1823 to bde6051 Compare December 19, 2016 23:17
@lasley
Copy link
Member Author

lasley commented Dec 19, 2016

Rebased, this is ready for review

@lasley lasley force-pushed the feature/master/certificate-request branch 3 times, most recently from 7611c68 to 92471a6 Compare December 19, 2016 23:27
@lasley lasley assigned tedsalmon and unassigned lasley Dec 20, 2016
Copy link
Contributor

@tedsalmon tedsalmon left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me! One minor issue and one suggestion in this review.

Thanks! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Imported but not used

Copy link
Member Author

Choose a reason for hiding this comment

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

Man it'll be nice once I finish with our MQT so we get Lint on non-Odoo

Copy link
Contributor

@tedsalmon tedsalmon Dec 20, 2016

Choose a reason for hiding this comment

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

I'm not sure if this is something you want to do, but you could remove a lot of code in this class and others by requiring these options to be sent in as keyword args.

class PolicyUse(object):

    def __init__(self, *args, **kwargs):
        self.__dict__.update(**kwargs)

a = PolicyUse(name='some_name', code='some_code')

print a.code

Output is:
some_code

Copy link
Member Author

Choose a reason for hiding this comment

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

I went this direction because everything is explicitly required. The whole point of these classes is to provide a data structure for usage, getting lazy when creating that structure just moves us back into what we had.

@lasley lasley force-pushed the feature/master/certificate-request branch from 92471a6 to 91033b1 Compare December 21, 2016 00:05
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 91033b1 on feature/master/certificate-request into 5418e0c on master.

@lasley lasley merged commit 693ebc2 into master Dec 21, 2016
@lasley lasley deleted the feature/master/certificate-request branch December 21, 2016 00:06
@lasley
Copy link
Member Author

lasley commented Dec 21, 2016

Updated, squashed, merged

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants