-
Notifications
You must be signed in to change notification settings - Fork 2
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
YNGJ-1058: Create Aether Observatory #297
base: main
Are you sure you want to change the base?
Conversation
A change to Portal catalog files was detected in your PR. Please visit this link to preview changes: https://portal-staging.powerapp.cloud/catalog?filters[kind]=all&filters[user]=all&filters[namespaceFilter]=yngj-1058-aether-observatory |
A change to documentation files was detected in your PR. Please visit this link to preview changes: https://portal-staging.powerapp.cloud/docs?filters[kind]=all&filters[user]=all&filters[namespaceFilter]=yngj-1058-aether-observatory |
|
||
spec.add_dependency "activemodel", ">= 6.0.6.1" | ||
spec.add_dependency "activesupport", ">= 6.0.6.1" | ||
spec.add_development_dependency "appraisal", "~> 2.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I see appraisal
here....we use it to help manage our rails updates and manage the dependencies different versions of rails. If properly installed, there should be an Appraisals file that declares versions (example here].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please add the appropriate gemfiles to the test matrix in your workflow. This library is highly dependent on the rails behavior, so I recommend you add the target rails versions to CI. As per our current usage of rails, I recommend you add rails 6.1, 7.0, 7.1 and 7.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks y'all. I meant to circle back to this, but forgot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
activemodel and activesupport are a part of rails core -- so appraising rails at a different version than the ones declared here is declaring two different versions of the same package. Since you don't directly rely on all of rails, just these two modules, you can update appraisal so it's just declaring these modules instead. We can find time tomorrow to work with you on it if needed.
We can also work with you on how we release this. We don't normally take the time to send our pre-releases to rubygems, since we can just point to the github version.
@that-jill and @xjunior I think I've got Appraisal set up correctly. Should it be testing all packages on all pushes? I tagged a version as v0.0.1pre4 that finally made it to rubygems. |
Use public interface rather than reaching into ActiveSupport::Notifications
6c3b831
to
b261766
Compare
Extracting
AetherObservatory
fromTalkbox
. Internal PubSub system base uponActiveSupport::Notifications