Skip to content
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

time: make time.Weekday data type public so that it can be pattern matched #57

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

bikallem
Copy link
Contributor

This PR makes time.Weekday exportable/public so that consumers of this package can pattern match on it. My use case is that I have to get the weekday of the the parsed PlainDateTime and do certain action based on the returned value.

At the moment I have to pattern match on a string value, like so

  let d = @time.PlainDateTime::of!(
    2000,
    2,
    29,
    hour=1,
    minute=10,
    second=30,
    nanosecond=1000,
  )

match d.weekday().to_string() {
"Monday" => ..
"Tuesday" => ...
"WednesDay" => ...
"Thursday" => ...
"Friday" => ...
"Saturday" => ...
"Sunday" => ...
_ => raise .. //

The d.weekday() value is already parsed and deemed to be valid, i.e. PlainDateTime.of is a success. It seems rather inconvenient to have to handle/pattern-match a case that we know will never occur. Exposing @time.Weekday as pub and exporting in time.mbti will remove this redundant match case. Also in a few instances I have found pattern matching on strings is rather fragile since there are chances of typos in the string which the compiler can't highlight during build time. Finally, Weekday values are rather stable (.i.e. they are not going to be changed, I think), so I think making it public should be quite future proof.

Copy link

peter-jerry-ye-code-review bot commented Sep 16, 2024

From the provided git diff output, here are three observations and suggestions:

  1. Redundant Definition of Weekday:

    • In the time/time.mbti file, the Weekday type is being redefined as a pub enum directly within the file. This might be redundant if the Weekday type is already defined in the time/weekday.mbt file, which is more appropriate as it seems to be specifically for managing weekdays. It's better to reference the Weekday type from the weekday.mbt file to avoid duplication and ensure consistency.
    • Suggestion: Remove the Weekday definition from time.mbti and ensure that it is only defined once in weekday.mbt.
  2. Public Accessibility of Weekday:

    • The Weekday enum in both files is marked as pub. This is correct if the intent is to make Weekday accessible from other modules or packages. However, if Weekday should be private to the time module, then the pub keyword should be removed.
    • Suggestion: Ensure that the accessibility of Weekday aligns with the intended module visibility. If it's meant to be used outside the time module, keep it as pub; otherwise, remove the pub keyword.
  3. Consistency in Naming Conventions:

    • The Weekday enum values are named using PascalCase, which is consistent with MoonBit's naming conventions for enum values. This is good practice as it enhances readability and adheres to standard coding conventions.
    • Suggestion: Continue using PascalCase for enum values and other identifiers to maintain consistency throughout the codebase.

These suggestions aim to improve code maintainability, reduce redundancy, and ensure consistency with the language's conventions.

@peter-jerry-ye
Copy link
Collaborator

@yj-qin What do you think?

@yj-qin
Copy link

yj-qin commented Sep 18, 2024

LGTM. It makes sense.

@peter-jerry-ye
Copy link
Collaborator

@bikallem Can you please rerun moon info and regenerate the mbti file?

@bikallem
Copy link
Contributor Author

@peter-jerry-ye Done.

@bikallem
Copy link
Contributor Author

Looks like CI failing due to CLA. Where/how can I sign it?

@Young-Flash
Copy link
Collaborator

Looks like CI failing due to CLA. Where/how can I sign it?

https://www.moonbitlang.com/cla

@Yoorkin
Copy link
Collaborator

Yoorkin commented Sep 23, 2024

Looks like CI failing due to CLA. Where/how can I sign it?

https://www.moonbitlang.com/cla

We don't require the contributor to sign a CLA anymore, so I just removed the CI check.

@Yoorkin Yoorkin merged commit ef37428 into moonbitlang:main Sep 23, 2024
7 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.

5 participants