Skip to content

Conversation

@dbgen1
Copy link
Contributor

@dbgen1 dbgen1 commented Feb 20, 2025

This PR only adds value checking to the RTC set time, date and alarm fields for ease of understanding when using. This will prevent things like proveskit/pysquared#164 from happening in the future.

@dbgen1 dbgen1 requested a review from Mikefly123 February 20, 2025 21:07
@sonarqubecloud
Copy link

if date < 1 or date > 31:
raise ValueError("Date value must be between 1 and 31")
if weekday < 0 or weekday > 6:
raise ValueError("Weekday value must be between 0 and 6")
Copy link
Member

Choose a reason for hiding this comment

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

Should we also have these statements check if the value passed is None or would it just not accept it if one of the values in the tuple that is sent in is just None.

Example:

c.rtc.set_date(99,None,29,5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello! I think it's a good idea to enforce that the full date be set, as I don't really see a use case for partial dates, and it can lead to weird behavior that the user would have to manage. Also, the plan is to eventually replace this format with the datetime iso8601 compliant format in the future, which will require a full date anyway.

Let me know if you have a specific use case for partial dates in mind though!

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good to me! I'll just rubber stamps this for now and we can circle back to full date checking in a future update.

@Mikefly123 Mikefly123 self-requested a review February 20, 2025 22:38
Copy link
Member

@Mikefly123 Mikefly123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@dbgen1 dbgen1 merged commit 97e7ac3 into main Feb 20, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants