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

Type Error, Random buffer, and Core #4327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Type Error, Random buffer, and Core #4327

wants to merge 1 commit into from

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Jan 28, 2025

Remove the steep ignore directive for 3 more files: datadog/core/error.rb, datadog/core/buffer/random.rb, and datadog/core.rb.

Change log entry
No

@marcotc marcotc requested a review from a team as a code owner January 28, 2025 23:39
@github-actions github-actions bot added the core Involves Datadog core libraries label Jan 28, 2025
# Steep doesn't like an array with up to 3 elements to be passed here: it thinks the array is unbounded.
when Array then new(*value)
# Steep can follow the logic inside the lambda, thus it doesn't know `value` responds to `:message`.
when ->(v) { v.respond_to?(:message) } then new(value.class, value.message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
when ->(v) { v.respond_to?(:message) } then new(value.class, value.message)
when ->(v) { v.respond_to?(:message) }new(value.class, value.message)
Don't use `then` for multi-line if, unless, when, or in statements (...read more)

The then keyword is not necessary in multi-line if/unless/when/in statements in Ruby. When used in multi-line statements, it can make the code harder to read and understand. This is because then is typically associated with single-line conditional statements in Ruby, and its use in multi-line statements can be confusing.

Maintaining readability and clarity in your code is crucial for effective collaboration and debugging. It becomes even more important in larger codebases, where complex logic can become difficult to follow if not written clearly.

To avoid this issue, omit the then keyword in your multi-line if/unless/when/in statements. For single-line if/unless/when/in statements, using then is acceptable and can help improve readability. This practice keeps your code clean and easy to understand, following the principles of good coding practices.

View in Datadog  Leave us feedback  Documentation

when ContainsMessage then new(value.class, value.message)
# steep:ignore:start
# Steep doesn't like an array with up to 3 elements to be passed here: it thinks the array is unbounded.
when Array then new(*value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
when Array then new(*value)
when Arraynew(*value)
Don't use `then` for multi-line if, unless, when, or in statements (...read more)

The then keyword is not necessary in multi-line if/unless/when/in statements in Ruby. When used in multi-line statements, it can make the code harder to read and understand. This is because then is typically associated with single-line conditional statements in Ruby, and its use in multi-line statements can be confusing.

Maintaining readability and clarity in your code is crucial for effective collaboration and debugging. It becomes even more important in larger codebases, where complex logic can become difficult to follow if not written clearly.

To avoid this issue, omit the then keyword in your multi-line if/unless/when/in statements. For single-line if/unless/when/in statements, using then is acceptable and can help improve readability. This practice keeps your code clean and easy to understand, following the principles of good coding practices.

View in Datadog  Leave us feedback  Documentation

@datadog-datadog-prod-us1
Copy link
Contributor

Datadog Report

Branch report: more-steep
Commit report: 6030823
Test service: dd-trace-rb

✅ 0 Failed, 22098 Passed, 1476 Skipped, 5m 8.44s Total Time

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.73%. Comparing base (8dba6cb) to head (6030823).

Files with missing lines Patch % Lines
lib/datadog/core/error.rb 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4327      +/-   ##
==========================================
+ Coverage   97.72%   97.73%   +0.01%     
==========================================
  Files        1368     1368              
  Lines       82997    82995       -2     
  Branches     4219     4219              
==========================================
+ Hits        81105    81113       +8     
+ Misses       1892     1882      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Jan 29, 2025

Benchmarks

Benchmark execution time: 2025-01-29 00:01:20

Comparing candidate commit 6030823 in PR branch more-steep with baseline commit 8dba6cb in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

Copy link
Member

@Strech Strech left a comment

Choose a reason for hiding this comment

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

I left a few notes to make the PR a bit more consistent

# steep:ignore:start
# Steep doesn't like an array with up to 3 elements to be passed here: it thinks the array is unbounded.
when Array then new(*value)
# Steep can follow the logic inside the lambda, thus it doesn't know `value` responds to `:message`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Steep can follow the logic inside the lambda, thus it doesn't know `value` responds to `:message`.
# Steep can not follow the logic inside the lambda, thus it doesn't know `value` responds to `:message`.

when Array then new(*value)
# Steep can follow the logic inside the lambda, thus it doesn't know `value` responds to `:message`.
when ->(v) { v.respond_to?(:message) } then new(value.class, value.message)
# steep:ignore:end
when String then new(nil, value)
Copy link
Member

Choose a reason for hiding this comment

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

We have Error.new lower, does it makes sense to have the same here or there?

@items: Array[Object]
@max_size: Integer

def initialize: (Integer)-> void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def initialize: (Integer)-> void
def initialize: (Integer) -> void


def add!: (Object) -> Object

def add_all!: (Array[Object])-> void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def add_all!: (Array[Object])-> void
def add_all!: (Array[Object]) -> void


def push: (Object) -> Object?

def replace!: (Object)-> Object?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def replace!: (Object)-> Object?
def replace!: (Object) -> Object?

@@ -2,6 +2,37 @@ module Datadog
module Core
module Buffer
class Random
@closed: bool
@items: Array[Object]
Copy link
Member

Choose a reason for hiding this comment

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

Does it makes sense to extract it as a type alias here and use in all the latter sigs in this file?

@p-datadog
Copy link
Member

LGTM, I also agree with the suggested changes

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

Successfully merging this pull request may close these issues.

4 participants