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

Finish localising the DateEntry and Calendar #5571

Merged
merged 15 commits into from
Mar 7, 2025
Merged

Finish localising the DateEntry and Calendar #5571

merged 15 commits into from
Mar 7, 2025

Conversation

andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Mar 2, 2025

Fixes #5141

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@coveralls
Copy link

coveralls commented Mar 2, 2025

Coverage Status

coverage: 62.322% (+0.01%) from 62.311%
when pulling 69f6bb6 on fix/5141
into 3c25ee0 on develop.

@andydotxyz
Copy link
Member Author

Thanks for adding the Sunday starts - but I don't think they all use US date format ...

@dweymouth
Copy link
Contributor

Thanks for adding the Sunday starts - but I don't think they all use US date format ...

I have them using the day/month/year format?

@andydotxyz
Copy link
Member Author

So you do - I can't read sorry.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

If you wish to add Swedish to the list then it is Monday and time.DateOnly that is correct.

@andydotxyz
Copy link
Member Author

If you wish to add Swedish to the list then it is Monday and time.DateOnly that is correct.

Thanks for this - but given that Swedish is the language do you mean Sweden? (i.e. (SE)?

This seems like a very memory inefficient data structure with quite a lot of duplicated data.

I have significantly reduced the memory requirement, hopefully this makes it good for now?

@Jacalz
Copy link
Member

Jacalz commented Mar 6, 2025

Thanks for this - but given that Swedish is the language do you mean Sweden? (i.e. (SE)?

Oh, yes of course. That's what I meant

Comment on lines +79 to +81
if pos := strings.Index(lang, "-"); pos != -1 {
region = strings.Split(lang, "-")[1]
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be cleaner to do using strings.Cut.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't that require the second instance of the separator? It is optional in this instance.

Copy link
Member

Choose a reason for hiding this comment

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

I figured it would be possible to do something like this to just use one strings API instead of two to achieve the same result. However, I do see now that the following code assumes that the region is the end of the file so it might be moot.

_, trailing, found := strings.Cut(lang, "-")
if found {
    region = trailing
}

@andydotxyz
Copy link
Member Author

Tests finally passing again

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Looking good 👍

@andydotxyz andydotxyz merged commit b65f1a5 into develop Mar 7, 2025
32 of 36 checks passed
@andydotxyz andydotxyz deleted the fix/5141 branch March 7, 2025 18:30
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