public Initializes of ACData
#6
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Maples7
reviewed
Apr 2, 2024
| let datas = date.datesInWeek.map { date -> ACData in | ||
| let data = ACData(date: date, count: getDateCount(sourceDates: sourceDates, date: date)) | ||
| return data | ||
| if let data = sourceDates[date] { |
There was a problem hiding this comment.
This change brings this bug #14 in. The date of sourceDates[date] doesn't necessarily need to be the start moment of the day. Therefore, sourceDates[date] is nil when toDate is Date().
There was a problem hiding this comment.
sorry my bad
- the dictionary was introduced because I had performance concerns.
- you need to use Date().startOfDay as a key to archive the count for that day
I made a mistake. this pr I didn't want to introduce these changes
only
-init(date: Date, count: Int = 0) {
+public init(date: Date, count: Int = 0) {
self.date = date
self.count = count
}
There was a problem hiding this comment.
@racechao That's ok, I've created a PR #13 to fix these issues.
- We can turn the
[Date]into a[Date:ACData]internally with only 1 literation (O(n)) by doing so 3270273#diff-6eace7fd00e64a79f64bb279676278b16eab8ec0f93e83fa4d8f5a422ffd040b, I think the performance should be acceptable. The difference is whether we need to request users to make the conversion by themselves. And we'd better not to break the API policy for backward compatibility. Or, we can create a newinitto support[Data:ACData]type parameter as thesourceDatasin the next new versions of this library. - Yeah the
datekeys have been turned intostartOfDayduring the process mentioned in the 1st point.
FYI @jasudev for more context.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.