Skip to content
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

Make setters private (rework of #69) #107

Merged
merged 7 commits into from Mar 25, 2015

Conversation

Projects
None yet
2 participants
@glennmatthews
Copy link
Contributor

commented Feb 18, 2015

Rework #69 to patch cleanly with latest master code.

Example code:

class COTInfo(COTGenericSubmodule):

    """Display VM information string."""

    def __init__(self, UI):
        """Instantiate this submodule with the given UI."""
        super(COTInfo, self).__init__(UI)
        self._package_list = None
        self._verbosity = None

    @property
    def package_list(self):
        """List of VM definitions to get information for."""
        return self._package_list

    @package_list.setter
    def package_list(self, value):
        for package in value:
            if not os.path.exists(package):
                raise InvalidInputError("Specified package {0} does not exist!"
                                        .format(package))
        self._package_list = value

    @property
    def verbosity(self):
        """Verbosity of information displayed."""
        return self._verbosity

    @verbosity.setter
    def verbosity(self, value):
        if value not in ['brief', 'verbose', None]:
            raise InvalidInputError(
                "Verbosity must be 'brief', 'verbose', or None")
        self._verbosity = value

Before - pep257 wants docstrings on setter methods:

> pep257 COT/info.py
COT/info.py:45 in public method `package_list`:
        D102: Docstring missing
COT/info.py:58 in public method `verbosity`:
        D102: Docstring missing
>

After - setters are ignored:

> /pep257 ../cot/COT/info.py
>
@glennmatthews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

This is technically an incomplete fix, as the same issue can exist with functions decorated as @foo.deleter - I'll look into that when I get a chance but in my experience setters are more common so this is valuable even in its current form.

@glennmatthews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

Looks like the original implementation covered deleters as well. I've just added a unit test to confirm this.

@glennmatthews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2015

Anything I can do to move this along? I can't adopt pep257 for my project without this fix:

$ pep257 COT
COT/add_disk.py:80 in public method `disk_image`:
        D102: Missing docstring in public method
COT/add_disk.py:96 in public method `address`:
        D102: Missing docstring in public method
COT/add_disk.py:111 in public method `controller`:
        D102: Missing docstring in public method
COT/add_file.py:62 in public method `file`:
        D102: Missing docstring in public method
COT/deploy.py:106 in public method `hypervisor`:
        D102: Missing docstring in public method
COT/deploy.py:121 in public method `configuration`:
        D102: Missing docstring in public method
COT/deploy.py:137 in public method `power_on`:
        D102: Missing docstring in public method
COT/deploy.py:260 in public method `ovftool_args`:
        D102: Missing docstring in public method
COT/edit_hardware.py:94 in public method `cpus`:
        D102: Missing docstring in public method
COT/edit_hardware.py:110 in public method `memory`:
        D102: Missing docstring in public method
COT/edit_hardware.py:150 in public method `nics`:
        D102: Missing docstring in public method
COT/edit_hardware.py:164 in public method `nic_type`:
        D102: Missing docstring in public method
COT/edit_hardware.py:174 in public method `serial_ports`:
        D102: Missing docstring in public method
COT/edit_properties.py:66 in public method `config_file`:
        D102: Missing docstring in public method
COT/edit_properties.py:78 in public method `properties`:
        D102: Missing docstring in public method
COT/help.py:52 in public method `subcommand`:
        D102: Missing docstring in public method
COT/info.py:53 in public method `package_list`:
        D102: Missing docstring in public method
COT/info.py:66 in public method `verbosity`:
        D102: Missing docstring in public method
COT/inject_config.py:62 in public method `config_file`:
        D102: Missing docstring in public method
COT/inject_config.py:84 in public method `secondary_config_file`:
        D102: Missing docstring in public method
COT/ovf.py:371 in public method `output_file`:
        D102: Missing docstring in public method
COT/ovf.py:491 in public method `system_types`:
        D102: Missing docstring in public method
COT/ovf.py:514 in public method `version_short`:
        D102: Missing docstring in public method
COT/submodule.py:131 in public method `package`:
        D102: Missing docstring in public method
COT/submodule.py:184 in public method `package`:
        D102: Missing docstring in public method
COT/submodule.py:206 in public method `output`:
        D102: Missing docstring in public method
COT/vm_description.py:121 in public method `output_file`:
        D102: Missing docstring in public method
COT/vm_description.py:180 in public method `system_types`:
        D102: Missing docstring in public method
COT/vm_description.py:189 in public method `version_short`:
        D102: Missing docstring in public method
COT/vm_description.py:198 in public method `version_long`:
        D102: Missing docstring in public method
COT/helpers/helper.py:220 in public method `name`:
        D102: Missing docstring in public method
@Nurdok

This comment has been minimized.

Copy link
Member

commented Mar 18, 2015

I will take a look at it over the weekend. Sorry for the delay.
On Mar 17, 2015 11:00 PM, "Glenn Matthews" notifications@github.com wrote:

Anything I can do to move this along? I can't adopt pep257 for my project
without this fix:

$ pep257 COT
COT/add_disk.py:80 in public method disk_image:
D102: Missing docstring in public method
COT/add_disk.py:96 in public method address:
D102: Missing docstring in public method
COT/add_disk.py:111 in public method controller:
D102: Missing docstring in public method
COT/add_file.py:62 in public method file:
D102: Missing docstring in public method
COT/deploy.py:106 in public method hypervisor:
D102: Missing docstring in public method
COT/deploy.py:121 in public method configuration:
D102: Missing docstring in public method
COT/deploy.py:137 in public method power_on:
D102: Missing docstring in public method
COT/deploy.py:260 in public method ovftool_args:
D102: Missing docstring in public method
COT/edit_hardware.py:94 in public method cpus:
D102: Missing docstring in public method
COT/edit_hardware.py:110 in public method memory:
D102: Missing docstring in public method
COT/edit_hardware.py:150 in public method nics:
D102: Missing docstring in public method
COT/edit_hardware.py:164 in public method nic_type:
D102: Missing docstring in public method
COT/edit_hardware.py:174 in public method serial_ports:
D102: Missing docstring in public method
COT/edit_properties.py:66 in public method config_file:
D102: Missing docstring in public method
COT/edit_properties.py:78 in public method properties:
D102: Missing docstring in public method
COT/help.py:52 in public method subcommand:
D102: Missing docstring in public method
COT/info.py:53 in public method package_list:
D102: Missing docstring in public method
COT/info.py:66 in public method verbosity:
D102: Missing docstring in public method
COT/inject_config.py:62 in public method config_file:
D102: Missing docstring in public method
COT/inject_config.py:84 in public method secondary_config_file:
D102: Missing docstring in public method
COT/ovf.py:371 in public method output_file:
D102: Missing docstring in public method
COT/ovf.py:491 in public method system_types:
D102: Missing docstring in public method
COT/ovf.py:514 in public method version_short:
D102: Missing docstring in public method
COT/submodule.py:131 in public method package:
D102: Missing docstring in public method
COT/submodule.py:184 in public method package:
D102: Missing docstring in public method
COT/submodule.py:206 in public method output:
D102: Missing docstring in public method
COT/vm_description.py:121 in public method output_file:
D102: Missing docstring in public method
COT/vm_description.py:180 in public method system_types:
D102: Missing docstring in public method
COT/vm_description.py:189 in public method version_short:
D102: Missing docstring in public method
COT/vm_description.py:198 in public method version_long:
D102: Missing docstring in public method
COT/helpers/helper.py:220 in public method name:
D102: Missing docstring in public method


Reply to this email directly or view it on GitHub
#107 (comment).

pep257.py Outdated
@@ -94,7 +94,8 @@ def __repr__(self):

class Definition(Value):

_fields = 'name _source start end docstring children parent'.split()
_fields = ['name', '_source', 'start', 'end', 'decorators', 'docstring',
'children', 'parent']

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

Make this a tuple instead (it shouldn't be mutable).

pep257.py Outdated
@@ -148,6 +150,10 @@ class Method(Function):

@property
def is_public(self):
# Check if we are a setter/deleter method, and mark as private if so.
for decorator in self.decorators:
if decorator.name.startswith(self.name):

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

Also check that after self.name there is a period, otherwise this will be considered private:

class Foo(object):
    @foobar
    def foo():
        pass

This comment has been minimized.

Copy link
@glennmatthews

glennmatthews Mar 23, 2015

Author Contributor

Great catch, thanks.

pep257.py Outdated
@@ -219,6 +232,7 @@ def __call__(self, filelike, filename):
self.stream = TokenStream(StringIO(src))
self.filename = filename
self.all = None
self._decorators = []

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

Maybe this should be named _current_decorators or something of this sort. At first I was confused as to why this is inside Parser.

This comment has been minimized.

Copy link
@glennmatthews

glennmatthews Mar 23, 2015

Author Contributor

Changed to _accumulated_decorators

pep257.py Outdated
def parse_decorators(self):
"""Called after first @ is found.
Build list of current decorators and return last parsed token,

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

This doesn't really return the last parsed token - the docstring should be updated.

pep257.py Outdated
self.current.value in ['def', 'class']):
break
elif self.current.kind == tk.OP and self.current.value == '@':
# New decorator found.

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

This comment confused me, because I didn't understand how a new decorator is appended if it's just been found out. State that you're saving the old one and resetting the parameters.

@@ -0,0 +1,242 @@
"""
Unit test for pep257 module decorator handling.

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

Put the one-liner text in the same line as the opening triple-quotes.

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

(Goes for the whole file)

@first_decorator
class Foo:
pass
"""

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

If you're using multi-line strings, indent them like the starting line and then use textwrap.dedent (it looks much better this way). You can look at test_pep257.py for examples.

This comment has been minimized.

Copy link
@glennmatthews

glennmatthews Mar 23, 2015

Author Contributor

Great suggestion, thanks.

[],
None,
all,
)

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 22, 2015

Member

This formatting is a bit weird... If you're putting one argument per line, at least use keyword arguments.

This comment has been minimized.

Copy link
@glennmatthews

glennmatthews Mar 23, 2015

Author Contributor

This was inherited from #69 - I agree it's atypical. Will clean it up.

@Nurdok

This comment has been minimized.

Copy link
Member

commented Mar 22, 2015

I added my code review as comments.
Other than that, please also add your change to the release notes in the docs, under the development version.

Thanks!

@glennmatthews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2015

Thanks for the thorough review! I'll address the comments and provide an updated changeset.

glennmatthews added some commits Mar 23, 2015

Merge pull request #1 from GreenSteam/master
Bump version to 0.5.0
pep257.py Outdated
@@ -116,7 +117,8 @@ def __str__(self):

class Module(Definition):

_fields = 'name _source start end docstring children parent _all'.split()
_fields = ['name', '_source', 'start', 'end', 'decorators', 'docstring',
'children', 'parent', '_all']

This comment has been minimized.

Copy link
@Nurdok

Nurdok Mar 24, 2015

Member

This should be a tuple too.

@Nurdok

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

@glennmatthews just one more tiny fix and I'll merge this :)

@glennmatthews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2015

Done. Thanks again! (And thanks to @adiroiban for the initial version of this enhancement)

Nurdok added a commit that referenced this pull request Mar 25, 2015

Merge pull request #107 from glennmatthews/master
Make setters private (rework of #69)

@Nurdok Nurdok merged commit 2b02b18 into PyCQA:master Mar 25, 2015

1 check passed

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2015

@Nurdok Can I expect a new release any time soon? I would like to enable pep257 on my project. 😄

@Nurdok

This comment has been minimized.

Copy link
Member

commented Jun 30, 2015

@glennmatthews I'm currently working on a #96 and a version will be released once I'm done. It might take 2-3 weeks, though.

@glennmatthews

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2015

Good enough for me. :-) Thanks for the reply!

Glenn

@glennmatthews glennmatthews referenced this pull request Jul 9, 2015

Closed

Make setters private #69

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.