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

Seperate sorbet sigs from rb files #80

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

donaldong
Copy link
Contributor

@donaldong donaldong commented Nov 10, 2020

This separates the sorbet sigs from rb files so sorbet runtime won't affect the lib code.

Test Plan

  • ci

Pull Request Branch: donaldong/release_v0_1_0
Pull Request Link: #79
Pull Request Branch: donaldong/seperate_sorbet_sigs_from_rb_files
Pull Request Link: #80
@donaldong donaldong changed the base branch from master to 929f91cc857cd89c6a1f9f2dcef0b75a951bc11e November 10, 2020 02:07
@donaldong donaldong force-pushed the donaldong/seperate_sorbet_sigs_from_rb_files branch from da95172 to b83fe2d Compare November 10, 2020 02:07
@donaldong donaldong requested review from katyho and a team November 10, 2020 02:09
@hdoan741
Copy link
Contributor

hdoan741 commented Nov 10, 2020

You can extends T::Sig::WithoutRuntime to keep the sig in the same file as the file but skip the runtime checks.

@donaldong
Copy link
Contributor Author

You can extends T::Sig::WithoutRuntime to keep the sig in the same file as the file but skip the runtime checks.

Actually, I think we're moving way from sorbet-runtime completely; and only use sorbet for static tc

@donaldong donaldong requested a review from hdoan741 November 12, 2020 01:50
@donaldong donaldong marked this pull request as draft September 22, 2021 15:58
@mattxwang mattxwang marked this pull request as ready for review August 15, 2022 20:27
@mattxwang mattxwang changed the base branch from 929f91cc857cd89c6a1f9f2dcef0b75a951bc11e to master August 15, 2022 20:27
@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #80 (2f3c00a) into master (aaa8550) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   96.95%   96.97%   +0.01%     
==========================================
  Files          35       35              
  Lines        1743     1553     -190     
==========================================
- Hits         1690     1506     -184     
+ Misses         53       47       -6     
Impacted Files Coverage Δ
lib/redcord/actions.rb 97.93% <ø> (+2.48%) ⬆️
lib/redcord/attribute.rb 98.46% <ø> (-0.35%) ⬇️
lib/redcord/base.rb 100.00% <ø> (ø)
lib/redcord/lua_script_reader.rb 100.00% <ø> (ø)
lib/redcord/migration.rb 100.00% <ø> (ø)
lib/redcord/migration/index.rb 86.66% <ø> (ø)
lib/redcord/migration/ttl.rb 100.00% <ø> (ø)
lib/redcord/redis.rb 100.00% <ø> (ø)
lib/redcord/relation.rb 98.70% <ø> (-0.27%) ⬇️
lib/redcord/serializer.rb 100.00% <ø> (ø)
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mattxwang
Copy link
Contributor

Note - to deal with the merge conflicts, I went through each merge file and moved T::sig changes to the private_rbi folder, trying to contextually understand what happened by looking at prior PRs. Think it's also now up to me to port over the rest of the changes to master.

@mattxwang
Copy link
Contributor

Hey @donaldong, just wanted to ask - what was the original purpose of this PR / how should I continue it, if at all? Katy and I were considering also completely removing sorbet from the project.

Alternatively, I can port over the original remove-sig logic to the new code that were introduced to master since this PR was opened! If that's the case, should I be:

  • removing all instances of sorbet in runtime code, including TypeCoerce, T::Struct, etc.?
  • or, just removing sigs, but keeping general runtime code

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

Successfully merging this pull request may close these issues.

3 participants