Added acceptance of string anchors/sides for TkInter such as "w" and "top"

[TRManderson]

TKInter accepts string arguments for anchor and sides, but our tests do not. This fixes our inflexibility so that students can write x.pack(side="left"), which Tk accepts.

[sapi]

This was actually intentional. Hard-coding string keywords is bad style, and I was hoping to discourage it. (I also suspect it might be deprecated in a version or two, seeing as PEP 435 has been accepted.)

For the same reason, I suspect some of the other tk tests might require tk.TRUE and tk.FALSE, though it's possible that I've been more flexible with the truthy constants.

My personal preference would be to require the constants, and perhaps to add some static analysis warnings explicitly telling students to use them (by checking for, eg, 'left'). However, if this is becoming a big issue, there's no harm in merging this.

If @pjritee uses string literals in the lectures, then this should definitely be merged.

[starsnabove]

On 21/04/15 15:39, Sean Purdon wrote:

This was actually intentional. Hard-coding string keywords is bad style, and I was hoping to discourage it. (I also suspect it might be deprecated in a version or two, seeing as PEP 435 https://www.python.org/dev/peps/pep-0435/ has been accepted.)

For the same reason, I suspect some of the other tk tests might require |tk.TRUE| and |tk.FALSE|, though it's possible that I've been more flexible with the truthy constants.

I concur

My personal preference would be to require the constants, and perhaps to add some static analysis warnings explicitly telling students to use them (by checking for, eg, |'left'|). However, if this is becoming a big issue, there's no harm in merging this.

If @pjritee https://github.com/pjritee uses string literals in the lectures, then this should /definitely/ be merged.

For above reasons I would hope that this is not happening

— Reply to this email directly or view it on GitHub https://github.com/CSSE1001/MyPyTutor/pull/158#issuecomment-94645297.

[TRManderson]

I just had a student point this out to me today. Peter isn't yet up to teaching GUIs, so we don't need to worry for now.

Perhaps the best course of action would be to modify my change to give a more detailed warning message when using "w" and "top", for example "You should use tk.TOP instead of {}"?

[sapi]

Yeah, that would probably be a good idea.

You could even generalise this somewhat (up to you whether it's worth the effort):

def validate_pack_arg(self, required, actual):
    _, _, required_arg = required.partition('.')

    if actual.lower() == required_arg.lower():
        self.add_warning(
            'You will need to specify your pack argument as {}, not {}'.format(
                required, actual
            )
        )

[TRManderson]

How do people feel about the messages below? I intend to do similar things in other places.

elif pack.keywords['anchor'] != 'tk.W':
    if pack.keywords['anchor'].lower() == "w":
        self.add_warning(
            'You appear to be using \'w\' as your anchor value. '
            ' (in {}). Please use tk.W'.format(pack.function_name)
        )
    else:
        self.add_error(
            'You appear to be using the wrong value for anchor '
            '(in {})'.format(pack.function_name)
    )

[sapi]

If you do it like that (if/else), then the first one needs to be an error, not a warning, otherwise the student will be able to submit without fixing the issue.

On 21 April 2015 at 18:54, Tom Manderson notifications@github.com wrote:

How do people feel about the messages below? I intend to do similar things in other places.

elif pack.keywords['anchor'] != 'tk.W': if pack.keywords['anchor'].lower() == "w": self.add_warning( 'You appear to be using \'w\' as your anchor value. ' ' (in {}). Please use tk.W'.format(pack.function_name) ) else: self.add_error( 'You appear to be using the wrong value for anchor ' '(in {})'.format(pack.function_name) )

— Reply to this email directly or view it on GitHub https://github.com/CSSE1001/MyPyTutor/pull/158#issuecomment-94708870.

[TRManderson]

That was intentional. Do you think error is the way to go? If so, I defer to the wisdom of the more experienced tutor and will do that in other cases.

[TRManderson]

Also, should we modify the expand check in GUI 3 so it only accepts tk.TRUE, or keep letting it accept True and 1?

[TRManderson]

Is this okay to be merged?