Should be able to distinguish unset from invalid time properties #98
Conversation
|
Looks like a fixed version of #73 would include this, can I help get it across the line? |
|
Hi @brackendawson I'm not sure @ManoloTonto1 is coming back so it might be a good idea to incorporate his changes into yours. (It also has a merge conflict.) The lint error this has is an easy fix with the solution in the error itself (package change.) I think the issue with that one was just the go-version. Happy to assist with the PR if you need it. I can create a chat if you have any questions. |
components.go
Outdated
| @@ -133,7 +133,7 @@ func (cb *ComponentBase) SetAllDayEndAt(t time.Time, props ...PropertyParameter) | |||
| func (cb *ComponentBase) getTimeProp(componentProperty ComponentProperty, expectAllDay bool) (time.Time, error) { | |||
| timeProp := cb.GetProperty(componentProperty) | |||
| if timeProp == nil { | |||
| return time.Time{}, errors.New("property not found") | |||
| return time.Time{}, ErrorPropertyNotFound | |||
There was a problem hiding this comment.
It's good, however we can probably make it better with:
return time.Time{}, fmt.Errof("%w: %s", ErrorPropertyNotFound. componentProperty)
As it enhances it. I am very much in support of these updates.
|
@brackendawson It seems I made a pull request into his pull request with my fixes for his issue (so I don't have to modify his commits etc.) ManoloTonto1#1 That would be a good place to start |
|
Thanks |
Hey @arran4 i will get back to this and the other PR later today. I have a stupidly busy day today 😩. Once i get home I will tackle these PR's 😉 |
|
Hey @ManoloTonto1 sorry to drop your name like that! Thanks! I'm not sure it's urgent but the promptness is appreciated! Speak soon! |
Co-authored-by: Arran Ubels <[email protected]>
brackendawson
force-pushed
the
time-prop-err
branch
from
4687bd8 to
ed74932
Compare
|
@ManoloTonto1 Will this create a merge conflict if I merge it a head of #73 did you want to pull the commits from this into yours? (Ensuring that @brackendawson 's commits still remain in history |
Given the new requirement for multiple error handling, I would bump it to 1.20 as per: #73 I am waiting on that before continuing with this. (Larger PRs are harder to update and review.) |
And test the target forever
| name: Test | ||
| permissions: | ||
| contents: read | ||
| strategy: | ||
| matrix: | ||
| go-version: ['1.14.15', '1.15.15', '1.16.15', '1.17.13', '1.22.3'] | ||
| go-version: ['oldstable', 'stable'] |
There was a problem hiding this comment.
My gut feeling is that this should match go.mod and then run to stable.
I didn't know it supported stable even!
There was a problem hiding this comment.
Unfortunately because one uses go-version: <version> and the other uses go-version-file: <path>, they can't live in the same matrix.
|
Hold up a minute, not all the matrix is being included in PR checks.. |
|
Ah, now they are: I'm happy with that. |
|
Took us a while. Do you need this version released or should we wait for the other PR? |
|
Thanks! I'm not in a hurry, so we can wait and do a release with all of the error handling at once. I can also skip ahead and pin a commit in my project if I want. |
|
All good. Night! |
Motivation
I have some error handling code after getting event end times and I want to distinguish events with no end time (fine) from events with invalid end times (not fine).
Changes