Earn extra honor and gain new allies!
Honor is earned for each new codewarrior who joins.
Learn more
  • I used Formatter because that's what I used to solve this Kata, avoiding using the name Format as required by the Kata!

    I then promptly forgot I had done that. But you are right, it's better, and reduces chances things clash. I've created a fork with that change. I also moved the b/c code to module level, out of the functions. I think I was being overly cautious there.

    The class-level attribute stays tags, however. 25 years of code wrangling makes me set in my ways like that. ;-)

  • aaah, wait! You wrote all of that based on my initial comment, where I totally mixed what was about the new/old stuff... x)

    I edited in the meantime and thought you didn't see the original version. => 4/5 of your answer match what I actually removed... x)

    • TAGS/tags: it's a class attribute, not an instance attribute, yes. And it's a constant. That's specifically the reason why I recommend CAPS.

    • About the compatibility...

      • In any case, don't duplicate the code uselessly. Do that at the very beginning of the test suite.
      • Ofc what I suggested works... And there are no UnboundLocalErrors.
      • Tho, with the solution setup using Format, that causes troubles, yes. But question actually is: why your solution is using the class Formatter while the solution setup is using Format? Formatter is a better name and avoids the clash about Format being a class instead of the old function.
  • Hi, thanks for looking! :-)

    if you provide the tags in the solution setup you should name the sturcture in CAPS (since it's a constant)

    It's a class attribute, not a global, so no, it should not be all-caps.

    the goal of moving to a class is to remove format from the solution setup. So why is it still there?

    The goal is to provide an object; in Python you generally would implement this as a class. It can be done as a metaclass but this is not that level of challenge.

    We could change the description to state that you are to write a class and that an instance of it will be created to test it, but I tried to work with the existing JavaScript pattern and not deviate so far as to require rewriting the description with conditional sections.

    the goal of moving to a class is to remove format from the solution setup. So why is it still there?

    Because the Kata description assumes the object instance already has been created. (And the goal of this fork was to remove the Python style violations present in the current Python translation, which presents the unwary Kata trainee with def Format(): ..., and nothing ever calls that function. It really sends those inexperienced with Python into the wrong direction altogether).

    you should put the backward compatibility stuff at the very beginning of the test suite.

    It is at the beginning, of the the two test functions. I can look into creating a global shim but this way is way thinner.

    the limitation to actually properly move to a class is in the sample tests. There, you need to use the correct formatting => no format stuff there and only the conventions matching the solution setup => Format. Hence, you can remove it from the solution setup.

    That'd conflict with the Kata description. There is no description of what the class interface looks like; how the class is called, what arguments it'll accept. I'm not going to introduce restrictions on that in a Kata that's already live.

    For the test cases part, as you want (sort of... See further):

    • either rewritte the whole test suite whith the new formatting and apply the backward compatibility toward the old naming
    • or keep the old conventions and apply the compatibility the other way around.

    I took the first option; I (severely) cleaned up the test suite. I needed to add an additional test for a corner case that the old didn't cover, and it was.. not great to work in what was there. So my fork uses proper Python style and niceties such as random.choices(), and functools.reduce() instead of repeated getattr() calls in a deep nested mess.

    About compatibility, simpler that way (assuming the tests are rewritten with the new convetions):

    You can't actually do that, for two reasons:

    • In a function, you can't treat a name as both a local and a global. I specifically avoid that issue by using glob = vars() and testing against that. Your suggested code would cause an UnboundLocalError for code that uses format, and not Format.

    • Code that uses both format and Format is not wrong and should not be penalised. E.g.:

      class Format:
          def __init__(self):
              super(Format, self).__init__()
          
      format = Format()`
      

    should work. It won't in your case, because replacing Format with format breaks the super() call.

    EDIT: about naming conventions, we generally don't care for parts that aren't visible to the user in the trainer. So no need to raise issues about that when you'll stumble on those katas (translates to: at least almost all the ones I translated/created x) -> I'm a fan of camelCase! XD )

    Sure, but it's all the nicer for future maintainers that want to improve things. Code conventions do matter!

    • if you provide the tags in the solution setup you should name the sturcture in CAPS (since it's a constant)
    • you should put the backward compatibility stuff at the very beginning of the test suite.

    About compatibility, simpler that way (assuming the tests are rewritten with the new convetions):

    try:
        Format=format
    except NameError:
      pass
    

    EDIT: about naming conventions, we generally don't care for parts that aren't visible to the user in the trainer. So no need to raise issues about that when you'll stumble on those katas (translates to: at least almost all the ones I translated/created x) -> I'm a fan of camelCase! XD )

  • Hi,

    It was my first time publishing a kata. I didn't really look at other katas and check if a similar one already existed.

  • Hi,

    Are you perfectly sure this one is different enough from all of those:

    because if the idea is to search for solutions and then do some maths, I guess we still can say it's a duplicate and you should unpublish it (we already have enough version of those)