Should detect Table.with_columns() and warn the user

[davidwagner]

A common error that I see from students is to write code like Table.with_columns('x', arr), when they should have used Table().with_columns('x', arr). I suggest we try to detect that.

I suspect we could add a check at the start of the with_column() method to issue an error if type(self)==str, or maybe more broadly if type(self)!=Table (as I don't think we expect anyone to subclass Table).

[adnanhemani]

This seems a bit harder than it looks. Since the number of arguments checked is on runtime, the TypeError caused when someone uses Table.with_columns(..., ...) is thrown on the caller's side. - it never actually makes it to the receiver's end. Thus, we can't actually write code in the function to check the type of self unless we make the arguments into un-named arguments (which I'm quite. sure we don't want). Any other ideas, @davidwagner?

[davidwagner]

with_columns uses vargs, so it seems like it should work:

    def with_columns(self, *labels_and_values, **formatter):

In particular, add something like this to the start of with_columns:

        if type(self)==str:
            raise TypeError('Use Table().with_columns() to create a new table, not Table.with_columns()')

or

        if not isinstance(self, Table):
            raise TypeError('Use Table().with_columns() to create a new table, not Table.with_columns()')

[adnanhemani]

OH, I was looking at with_column rather than with_columns! So this is completely possible with with_columns, but not with with_column - but I'd say that the behavior should be identical (in relation to the warning due to the relationship between the methods). However, as noted above, this wouldn't be possible.

Do you think it'd be okay to put a check on with_columns and not with_column?

[papajohn]

Asking students to distinguish between with_column and with_columns didn’t work, last time I taught. Anything we can do to make those methods synonyms would be a net benefit, I think.

…

On Wed, Jul 3, 2019 at 1:09 AM Adnan Hemani ***@***.***> wrote: OH, I was looking at with_column rather than with_columns! So this is completely possible with with_columns, but not with with_column - but I'd say that the behavior should be identical (in relation to the warning). However, as noted above, this wouldn't be possible. Do you think it'd be okay to put a check on with_columns and not with_column? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#333?email_source=notifications&email_token=AACOFELC64WE5P4NVWTUYOLP5QYBDA5CNFSM4FWVZHN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZDI3OQ#issuecomment-507940282>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACOFELDGBM5PYZ7Z45IBC3P5QYBDANCNFSM4FWVZHNQ> .

[davidwagner]

Yes, it's fine to put the check on with_columns and not with_column. This isn't changing any semantics or behavior; it's just adding a warning about one common student error. If we can't warn for all student errors, it's still useful to warn for some of them.