Beta

Pokémon Types [Pokémon OOP #1]

14ffdevv
Description
Loading description...
Object-oriented Programming
View
AllIssues5QuestionsSuggestionsShow Resolved
  • Please sign in or sign up to leave a comment.
  • Voile Avatar

    It is not tested that the instance attribute is really defined as name, only accidentally (because part of random tests explode if it isn't)

    • ffdevv Avatar

      IMHO the fact that the tests (which are not random) failed with this exact message AttributeError: 'PokemonType' object has no attribute 'name' in case name was not implemented clearly mean that the thing is tested for, and this should be marked as a Suggestion, not an issue. Btw I now have added 2 more explicit test cases.

      Issue marked resolved by ffdevv 2 years ago
  • Blind4Basics Avatar

    Hi,

    The dictionary contains all the needed data for the interactions between types

    Are you sure about that...?

    test.expect(PT.get("Ghost").is_immune_to("Normal"))
('Flying', {'Fight': 2, 'Rock': 0.5, 'Bug': 2, 'Steel': 0.5, 'Grass': 2, 'Electric': 0.5})
=> no "Normal" in there while the default multiplier leads to "Not resistant" instead of "Immune"
    • sample tests are not using the test framework properly (...again)
    • I guess it's the same with the full test suite?
    • DON'T use test.expect unless you provide a useful error message. Tip: just use test.assert_equals...
    • the rules about the DefaultType are never explained anywhere (that's full specs. The user shouldn't have to guess what's on your mind)

    Cheers

    • ffdevv Avatar

      Hi, this time (to answer the "...again") I don't think you nail any real point tbh.

      1. the relevant part you provide shows details for 'Flying' class when you comment a test that expects 'Ghost' to be immune to 'Normal'. So, to answer you, yes this line is present in the dictionary: "Normal": {"Ghost": 0} i.e. "Normal" does 0 damage to "Ghost".
      2. Sample tests were really readable, concise and I didn't feel it needed extra verbosity because the users can add it if they feel they need it.
      3. You should remove the stroke from the "I guess" part because there isn't (and wasn't) any test.expect nor test.assert_equals without error description in the internal test case. If you found some other type of misleading error description please provide me something to watch for and I'll fix it
      4. As said at point 2, imho test.expect(PT.get("Ghost").is_immune_to("Normal")) in the sample tests where users can add/comment out/edit as they need to (if they actually experience errors) is perfectly fine, readable and clear. I also don't think test.assert_equals was properly used, as per your observation, since it missed the verbose description. I've still added message description for every error (both expect and assert_equals) in the example test case, althought they make the test case code box less clean and more annoying to edit for the users imo, but whatever.
      5. The DefaultType was declared this way DefaultType = TypeVar("DefaultType") meaning 'DefaultType is a generic type with name "DefaultType"'. I've still changed it to DefaultType = TypeVar("T") to be even more clear that it's just a generic type used to specify that the return type of get should be either PokemonType or any type the default param is. I think that what's on my mind is also on the initial solution, so it doesn't really need to be verbosely explained. Do you really think I should explain this inside the description?

      PS I still wait for your response to mark the issue as solved

    • Blind4Basics Avatar
      1. oops, right, copy/paste mistake, sorry. Cannot get my hand on the original assertion (I changed my code in the meantime), but basically, the dict is "directed" (ex: flying doesn't contain info about ground, but the opposite is true). So my point stil stands: the initial data structure is not consistent.
      2. This has nothing to do with readability. Writing the tests without proper use of test framework is just a "no" for the approval of the kata. And as the author of the kata you are responsible for writing it properly. Otherwise, there is no point in authoring kata.
      3. the stroke is definitely there, because point 3 refers to point 2 which refers to the use of the test framework (which, in present context, refers to the use of @test.desribe & @test.it decorator. I realize now you mixed this up with the assertion thing (which is also a problem, but not the same. My mistake again).
      4. ok, I see. Please note, tho:
      • writing the tests in different fahsion (full test suite vs example tests) has a cost in maintainability => when possible, the example tests should just be a copy paste of some initial part of the full test suite.
      • if the user sees test.expect in the example tests, they will assume the invisible tests will be written that way. That doesn't encourage in thinking they'll have a good experience with the kata.
      1. You didn't understand what I meant. I was talking about the behaviours related to the presence or not of a value for the default argument. there you only talked about implementation details, which are effectively already visible in the code. The problem is more about:
      • why None as default instead of, for example, "Normal"?
      • what is the user supposed to do when they get None as default?
        • throw an error?
        • use something else?
        • is it guaraanteed to never happen? (in that case, it must be specified and it would be better to not use None as default value)
    • ffdevv Avatar
      1. The constant is labeled ATTACK_MULTIPLIERS so it is consistent that it contains details from an attacking POV. Flying attacks Ground with 1x damage (nothing to specify), while Ground attacks Flying with a 2x damage. This is not always the case that if Type_A does 2x dmg Type_B then Type_B does 0.5x dmg Type_A.
      2. Ok, I didn't want to argue on the fact that test should be done properly, but that I've found others approved kata with just 3 or 4 lines of tests (on the samples) and I really appreciated them more because of the freedom and accessibility they provided than formally long tests samples not so easy to edit. Thus IMO, inside the test sample is a proper use (or at least a permitted one, since there are katas with high satisfaction votes out there that use just test.expect something) of the testing suite
      3. I think I still don't get it: in the full test suite all the tests are implemented with decorators and everything, eg
        @test.describe("Configuration Checks")
def tests():
 @test.it("All types are correctly initialized")
 def it():
      4. So people do actually prefer to get their mind-cpu busy assuming instead of just trying to press submit and see what happens? (not a salty question) I personally always try test AND submit with the real first attempt I do to solve a kata (or to have info without bothering to look back at the description). Btw I've integrated your suggestions on this (and the related point 2)
      5. Ok I see, few additional words in the description can be worth to be integrated to explain that detail.
      get(name, default = None): Get a single type instance
-->
get(name, default = None): Get a single type instance. If no instance is found returns either 'None' or whatever the consumer has provided via the optional param 'default'.
      Issue marked resolved by ffdevv 2 years ago
  • Fbasham Avatar

    I'm pretty sure your Bug/Fairy multipliers are wrong. I'm doing my own implmentation of the multiplier and only failing on Bug/Fairy comparisons. Check to see if your're missing something or have incorrect values.

    Also: Bug/Dark.

    • ffdevv Avatar

      Nice catch, pretty coherent the bugs were in the Bug isn't it? Fixed, thank you. I have also double checked others and didn't find any additional error.

      Issue marked resolved by ffdevv 2 years ago
  • Mercy Madmask Avatar

    AttributeError: 'PokemonType' object has no attribute 'name'

    It doesn't say in the description that we need to store the name as an attribute, only implement the methods.

  • Fbasham Avatar

    You're demanded to implement the PokemonType class implementing all the methods provided in the initial solution, according to their docstring and the type hinting.

    Sorry, but this isn't how it should be done. Specifications should be in the kata description. A user shouldn't have to click the train button to uncover what they need to do.