Skip to content

Conversation

hojatm-huma
Copy link
Contributor

add more clarification

add more clarification
@gyermolenko
Copy link
Contributor

"add more clarification" PR description is misleading. You change example itself.
As for the code itself:

  1. If class has only one method (as in on_press or execute) and no state - it is a candidate for a function. It doesn't mean the example is bad or good. Just not "pythonic"?
  2. Current implementation shows popular use case for Commands - possibility to store them and perform "undo".
  3. Once again, I don't like abstract classes for such examples.

Please take these comments just as an opinion and a ground for discussion.

A simple command to bold a text.
"""

def execute(self):
Copy link
Owner

@faif faif Mar 12, 2020

Choose a reason for hiding this comment

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

To show the effect of the command it is good to have some definition of bold. Maybe in this case wrap the text with the HTML <b> or <strong> element. The same goes for italic.

@hojatm-huma hojatm-huma changed the title Update commad pattern changing description and example to add more clarification Mar 14, 2020
@hojatm-huma
Copy link
Contributor Author

Hi guys!
Would you please check the code?

@gyermolenko @faif

def rename(self, src, dest):
print("renaming {} to {}".format(src, dest))
os.rename(src, dest)
def on_do_press(self, filename):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think names like on_do_press and on_undo_press are harder to read then rename, delete or whatever else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but the invoker (here is the menu item) does not know what is going to do.

Am I right?

@gyermolenko
Copy link
Contributor

Can we think of example without actual os files manipulations? Deleting files is a bit dangerous and not really necessary here.

@faif faif merged commit 697deba into faif:master Mar 17, 2020
@hojatm-huma hojatm-huma deleted the patch-1 branch March 19, 2020 07:53
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