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

Add types to FileUpdaters::Base #8214

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Conversation

JamieMagee
Copy link
Contributor

@JamieMagee JamieMagee commented Oct 17, 2023

A couple of notable things I'd like to highlight about this PR:

  • Reordered the parameters of initialize to fix Sorbet/KeywordArgumentOrdering: Optional keyword arguments must be at the end of the parameter list
  • It's a little unclear what the types of credentials and options should be
    • I was more confident in credentials being T::Hash[String, String]
    • I was less confident in options so it's T::Hash[String, T.untyped]
  • Methods marked with overridable should likely be marked as abstract, but abstract methods are required to have no body, and I am not sure if anything is depending on them raising a NotImplementedError
  • Changes to other files are type system 'escape hatches' as they haven't been properly typed yet and I wanted to keep this PR small

Part of #7782

@github-actions github-actions bot added L: go:modules Golang modules L: dotnet:nuget NuGet packages via nuget or dotnet L: java:maven Maven packages via Maven labels Oct 17, 2023
@JamieMagee JamieMagee marked this pull request as ready for review October 17, 2023 04:09
@JamieMagee JamieMagee requested a review from a team as a code owner October 17, 2023 04:09
@JamieMagee JamieMagee force-pushed the jamiemagee/file_updater_base branch 3 times, most recently from 2ee83dc to 5f15657 Compare October 18, 2023 15:45
@JamieMagee JamieMagee force-pushed the jamiemagee/file_updater_base branch from 5f15657 to ce2a9d4 Compare October 18, 2023 21:35
Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

Since it's a fundamental class I'll deploy it to see if there's any runtime errors before merging.

@jakecoffman jakecoffman merged commit 9a8a26f into main Oct 19, 2023
97 checks passed
@jakecoffman jakecoffman deleted the jamiemagee/file_updater_base branch October 19, 2023 14:17
@JamieMagee JamieMagee added the sorbet 🍦 Relates to Sorbet types label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet L: go:modules Golang modules L: java:maven Maven packages via Maven sorbet 🍦 Relates to Sorbet types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants