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

activejob: Add types for ActiveJob::Logging #732

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sanfrecce-osaka
Copy link
Contributor

  • logger is defined using Class#cattr_accessor in activesupport with instance_reader/instance_writer/instance_accessor set to true.
    • The default for logger is ActiveSupport::TaggedLogging, but it is not necessarily an instance of ActiveSupport::TaggedLogging or an inheritor of ActiveSupport::TaggedLogging (e.g. lograge). Therefore, it is defined as untyped.
  • log_arguments is defined using Class#class_attribute in activesupport with instance_accessor set to false and instance_predicate set to true.

- `logger` is defined using `Class#cattr_accessor` in activesupport with instance_reader/instance_writer/instance_accessor set to true.
  - The default for `logger` is ActiveSupport::TaggedLogging, but it is not necessarily an instance of ActiveSupport::TaggedLogging or an inheritor of ActiveSupport::TaggedLogging (e.g. lograge). Therefore, it is defined as untyped.
- `log_arguments` is defined using `Class#class_attribute` in activesupport with instance_accessor set to false and instance_predicate set to true.
Copy link

@sanfrecce-osaka Thanks for your contribution!

Please follow the instructions below for each change.
See also: https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md

Available commands

You can use the following commands by commenting on this PR.

  • /merge: Merge this PR if CI passes

activejob

You changed RBS files for an existing gem.
You need to get approval from the reviewers of this gem.

@ksss, @tk0miya, please review this pull request.
If this change is acceptable, please make a review comment including APPROVE from here.
Screen Shot 2024-03-19 at 14 13 36

After that, the PR author or the reviewers can merge this PR.
Just comment /merge to merge this PR.

Comment on lines +35 to +37
def log_arguments: () -> bool
def log_arguments=: (bool value) -> bool
def log_arguments?: () -> bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly, .log_arguments was added in v6.1.
rails/rails@ce085f6

Therefore it's not good to add here. Please add 6.1/ or newer directory if you need to use this method.

Comment on lines +33 to +34
def logger: () -> untyped
def logger=: (untyped val) -> untyped
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: The API doc says:

Accepts a logger conforming to the interface of Log4r or the default Ruby Logger class. You can retrieve this logger by calling logger on either an Active Job job class or an Active Job job instance.
https://api.rubyonrails.org/classes/ActiveJob/Logging.html#method-i-logger

I'm not sure "the interface of Log4r". So I'm okay to keep this untyped.

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

Successfully merging this pull request may close these issues.

2 participants