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

Integrate Sorbet type checking #404

Closed
bensheldon opened this issue Oct 5, 2021 · 10 comments · Fixed by #760
Closed

Integrate Sorbet type checking #404

bensheldon opened this issue Oct 5, 2021 · 10 comments · Fixed by #760
Projects

Comments

@bensheldon
Copy link
Owner

It would be nice to integrate Sorbet. I have tried before in #235, but found more sharp edges than I was comfortable with.

Short-term success is having some additional manual tooling and workflows that adds value to the development process and is not burdensome to contributors.

Long-term success would be to run Sorbet type checking as part of the CI lint process (e.g. there is a clean/green state and deviation from that state would fail CI).

@bensheldon bensheldon added good first issue Good for newcomers hacktoberfest Issues that are good for Hacktoberfest participants labels Oct 5, 2021
@toshitapandey
Copy link

@bensheldon - May I give it a try?

@bensheldon
Copy link
Owner Author

@toshitapandey definitely! 🎉 I'm available to answer any questions or help navigate. Ask questions here or ping me on https://www.rubyonrails.link/

@bensheldon
Copy link
Owner Author

@toshitapandey just wanted to check in and see if can help. I definitely want to encourage you to prioritize an incomplete PR to discuss over perfection. Thank you!

@bensheldon bensheldon added this to Inbox in Backlog Nov 25, 2021
@saksham-jain
Copy link
Contributor

Hey, @bensheldon saw this issue is open for a long time. If someone has not taken it up already can I take it up?
Also need some more description on the same as I am new to this project.

@bensheldon
Copy link
Owner Author

@saksham-jain thank you for the interest! unfortunately I don't think this is a good first issue unless you've set up sorbet before. I should remove that tag.

What sort of thing are you looking to help on?

@bensheldon bensheldon removed good first issue Good for newcomers hacktoberfest Issues that are good for Hacktoberfest participants labels Jul 20, 2022
@saksham-jain
Copy link
Contributor

@bensheldon Yes sorbet would be new to me. I would like to start contributing to this project with beginner-friendly issues/features. Once gets good understanding of the project can even pick bigger issues.

@bensheldon
Copy link
Owner Author

@saksham-jain Do you want to take #673 ?

@saksham-jain
Copy link
Contributor

saksham-jain commented Jul 21, 2022

Sure @bensheldon. Please assign me that issue.

@dixpac
Copy link
Contributor

dixpac commented Jul 9, 2023

I think cons will overweight the pros here for good_job at the current time and size of the engine.

Maybe introducing sorbet without all the type definitions could be beneficial to some degree(ex: catching NameErrors) but going full Sorbet I feel will slow down the development process, and make the pool of potential contributors smaller.

@bensheldon
Copy link
Owner Author

@dixpac having gotten #760 to green, I think we're pretty closely aligned.

I'd like GoodJob to be tapioca-able in the parent application (and this repo be srb tc passing) but I'm not planning to add explicit type definitions to GoodJob (eg not typed: strict). And I don't even think I can get GoodJob to typed: true without runtime type hinting, which I'm ignorantly reluctant to do.

As for contribution-friendliness, I realize that potential devs may self-select never to open a PR if they think they can't carry it to completion, but that's not how I maintain this repo. I love for folks to contribute what they can and then I can assist it over the finish line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Backlog
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants