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.

Sign up for GitHub

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

Jump to bottom

[BUG]: Inconsistent types on WebHook models #524

Closed
1 task done
ilmax opened this issue Jul 2, 2024 · 3 comments · Fixed by #525
Closed
1 task done

[BUG]: Inconsistent types on WebHook models #524

ilmax opened this issue Jul 2, 2024 · 3 comments · Fixed by #525
Labels
Type: Bug Something isn't working as documented

Comments

@ilmax
Copy link

ilmax commented Jul 2, 2024

What happened?

Different types exposes similar properties using different underlying types, I was looking at the *_at properties in the class
WorkflowJob and seems like those properties are exposed as string while most of the time they're exposes as DateTimeOffset.

Having a quick look here I found some more occurrences here and here

Can we unify those and use DateTimeOffset everywhere? This will be a breaking change so it requires a major version bump.

Versions

2.2.2

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@ilmax ilmax added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Jul 2, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 2, 2024

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@JamieMagee JamieMagee mentioned this issue Jul 2, 2024
Merged
4 tasks
@JamieMagee JamieMagee removed the Status: Triage This is being looked at and prioritized label Jul 2, 2024
@JamieMagee
Copy link
Contributor

@ilmax thanks for opening the bug. I think #525 resolves this, but I agree that this is a breaking change. I'll see if we have any more immediate breaking changes that we want to batch together into a 3.0.0 release.

@ilmax
Copy link
Author

ilmax commented Jul 2, 2024

Looking forward to version 3 soon then, thank you for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Type: Bug Something isn't working as documented
Projects
🧰 Octokit Active
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use DateTimeOffset everywhere instead of string octokit/webhooks.net
2 participants
@JamieMagee @ilmax