Skip to content

last_date_to_pay field can be none #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

SL1994-design
Copy link
Contributor

@SL1994-design SL1994-design commented Nov 27, 2024

User description

add none as default values to some fields

What changes do you are proposing?

How did you test these changes?

Closing issues


PR Type

enhancement


Description

  • Updated the ElectricBill dataclass to allow the last_date_to_pay field to be None by using Optional typing and setting a default value of None.
  • Modified the ResponseDescriptor and ResponseWithDescriptor dataclasses to set default values of None for code, description, and data fields, enhancing the flexibility of these models.

Changes walkthrough 📝

Relevant files
Enhancement
electric_bill.py
Allow `last_date_to_pay` field to be None                               

iec_api/models/electric_bill.py

  • Added Optional typing to last_date_to_pay.
  • Set default value of last_date_to_pay to None.
  • +3/-2     
    response_descriptor.py
    Set default None values for response fields                           

    iec_api/models/response_descriptor.py

  • Set default values of code and description to None.
  • Set default value of data to None.
  • +3/-3     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    add none as default values to some fields
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Data Validation
    The last_date_to_pay field is now optional but there's no validation logic to handle None values in downstream code that may expect a valid date string

    Code Organization
    The data field is moved after response_descriptor which may affect serialization order. Consider keeping consistent field ordering

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Use appropriate date type instead of string for date fields to ensure proper date handling and validation

    The last_date_to_pay field is defined as Optional[str], but since it represents a
    date, it should use a more appropriate type like Optional[datetime] or
    Optional[date] to ensure proper date handling and validation.

    iec_api/models/electric_bill.py [52]

    -last_date_to_pay: Optional[str] = field(metadata=field_options(alias="lastDateToPay"), default=None)
    +last_date_to_pay: Optional[datetime] = field(metadata=field_options(alias="lastDateToPay"), default=None)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using datetime instead of str for date fields is a good practice that enables proper date validation and manipulation. This change would improve type safety and make the code more maintainable.

    7
    Place optional fields after required ones in dataclass definitions for better code organization

    The data field should be placed before response_descriptor to maintain a logical
    order where optional fields come after required ones, following Python's dataclass
    best practices.

    iec_api/models/response_descriptor.py [37-38]

    +data: Optional[T] = None
     response_descriptor: ResponseDescriptor = field(metadata=field_options(alias=RESPONSE_DESCRIPTOR_FIELD))
    -data: Optional[T] = None
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While the suggestion follows Python's dataclass best practices, the current field order doesn't significantly impact functionality or maintainability. The improvement is mainly cosmetic.

    3

    💡 Need additional feedback ? start a PR chat

    @@ -11,8 +11,8 @@ class ResponseDescriptor(DataClassDictMixin):
    """Response Descriptor"""

    is_success: bool = field(metadata=field_options(alias="isSuccess"))
    code: Optional[str]
    description: Optional[str]
    code: Optional[str] = None
    Copy link
    Owner

    Choose a reason for hiding this comment

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

    No reason for setting default values, they'll be None by default

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I had some errors without this added line. for example: errorMessage": "Field \"response_descriptor\" of type ResponseDescriptor in ResponseWithDescriptor has invalid value {'isSuccess': True, 'code': '00'}",

    Copy link
    Owner

    Choose a reason for hiding this comment

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

    When trying to parse the response:

    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<string>", line 6, in __mashumaro_from_dict__
    mashumaro.exceptions.MissingField: Field "data" of type Optional[Any] is missing in ResponseWithDescriptor instance
    

    This means that data field (and not code) can be None.
    Which you already added.

    @GuyKh
    Copy link
    Owner

    GuyKh commented Nov 28, 2024

    @SL1994-design - please fix the lint comments

    @GuyKh
    Copy link
    Owner

    GuyKh commented Dec 1, 2024

    @SL1994-design - please fix the lint comments

    iec_api/models/electric_bill.py:55:61: W292 [*] No newline at end of file
       |
    55 | decoder = BasicDecoder(ResponseWithDescriptor[ElectricBill])
       |                                                              W292
       |
       = help: Add trailing newline
    
    iec_api/models/response_descriptor.py:38:29: W292 [*] No newline at end of file
       |
    37 |     response_descriptor: ResponseDescriptor = field(metadata=field_options(alias=RESPONSE_DESCRIPTOR_FIELD))
    38 |     data: Optional[T] = None
       |                              W292
       |
       = help: Add trailing newline
    
    

    Please add newlines at the end of the files

    @SL1994-design SL1994-design requested a review from GuyKh December 2, 2024 19:48
    @GuyKh
    Copy link
    Owner

    GuyKh commented Dec 3, 2024

    @SL1994-design - please do it for both files.

    Copy link
    Owner

    @GuyKh GuyKh left a comment

    Choose a reason for hiding this comment

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

    Please see lint errors.

    @GuyKh GuyKh merged commit ac4bc03 into GuyKh:main Dec 3, 2024
    1 check passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants