Skip to content

Converted a few class variables into instance variables#284

Merged
tleonhardt merged 3 commits intomasterfrom
instance_variables
Feb 27, 2018
Merged

Converted a few class variables into instance variables#284
tleonhardt merged 3 commits intomasterfrom
instance_variables

Conversation

@tleonhardt
Copy link
Copy Markdown
Member

@tleonhardt tleonhardt commented Feb 24, 2018

Now that users can nest instances of cmd.Cmd2 to support creating sub-menus, we should need to be more careful about class vs instance variables to prevent potential problems.

This converts the following former class variables into instance variables:

  • multiline_commands
  • shortcuts
  • exclude_from_help
  • exclude_from_history

In the process, a couple camelCase variable names got converted to pep8_compliant names.

There may be a few other class variables which should be converted to instance variables. But at the very least, this is a good start.

Now that users can nest instances of cmd.Cmd2 to support creating sub-menus, we should need to be more careful about class vs instance variables to prevent potential problems.

This converts the following former class variables into instance variables:
- multiline_commands
- shortcuts
- exclude_from_help
- exclude_from_history

In the process, a couple camelCase variable names got converted to pep8_compliant names.

There may be a few other class variables which should be converted to instance variables.  But at the very least, this is a good start.

This closes #273.
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2018

Codecov Report

Merging #284 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #284   +/-   ##
=======================================
  Coverage   91.57%   91.57%           
=======================================
  Files           1        1           
  Lines        1424     1424           
=======================================
  Hits         1304     1304           
  Misses        120      120
Impacted Files Coverage Δ
cmd2.py 91.57% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27a8a15...91255cd. Read the comment docs.

Copy link
Copy Markdown
Contributor

@kotfu kotfu left a comment

Choose a reason for hiding this comment

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

It appears that this would be a breaking change to all subclasses which are using multilineCommands or excludeFromHistory or exclude_from_help or shortcuts. Breaking meaning if I install the soon to be release cmd2 version with these changes, my app won't run anymore until I fix it to use the new variable names. Did I understand the impact of this change correctly?

If so, perhaps we should consider doing something like creating these variable names as properties, so that both the old name and the new name work. That way we can deprecate the old names, but still have them work, to give developers time to modify their code.

@tleonhardt
Copy link
Copy Markdown
Member Author

tleonhardt commented Feb 27, 2018

You make a good point. On second thought, maybe changing the two attribute names isn't such a hot idea. It would even break our own examples.

I think the change would be dramatically less breaking if I didn't rename those two.

I don't view the act of making these attributes instance members instead of class members to be particularly breaking, especially since I think all of our examples reference them via self anyways.

I think I'll change those back to their original names.

Edit: On second thought, I think I am going to roll back the shortcut and multilineCommand changes as well since that could be breaking to a lot of apps and probably doesn't add a lot of value for common usage when using sub-menus.

…story to prevent breaking change

Also:
- Reverted multilineCommands and shortcuts to class variables to prevent other breaking changes
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.

2 participants