Skip to content

ref(send-event): Introduce constant for max breadcrumbs - #2716

Merged
szokeasaurusrex merged 1 commit into
masterfrom
szokeasaurusrex/max-breadcrumbs-constant
Sep 12, 2025
Merged

ref(send-event): Introduce constant for max breadcrumbs#2716
szokeasaurusrex merged 1 commit into
masterfrom
szokeasaurusrex/max-breadcrumbs-constant

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

While investigating #2709, I noticed the max breadcrumbs limit was hardcoded in multiple locations. This change extracts the value into a constant, so it is easier to change later, if we ever need to.

While investigating #2709, I noticed the max breadcrumbs limit was hardcoded in multiple locations. This change extracts the value into a constant, so it is easier to change later, if we ever need to.
@szokeasaurusrex
szokeasaurusrex requested a review from a team as a code owner August 26, 2025 09:36
@szokeasaurusrex
szokeasaurusrex enabled auto-merge (squash) August 26, 2025 09:36

@lcian lcian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good.
I think it would be better to define it only in one place but I think I understand why you did it like this. If you later want to make this a CLI argument, you would have to add it in both places anyways

@szokeasaurusrex
szokeasaurusrex merged commit a031cfd into master Sep 12, 2025
31 checks passed
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/max-breadcrumbs-constant branch September 12, 2025 09:37
@szokeasaurusrex

Copy link
Copy Markdown
Member Author

Yeah, I think that is exactly why I did it this way, as each command should define its limit separately. Also, bash-hook is not super maintained (I'd say it's basically soft-deprecated at this point), which is another reason why I did not put too much effort into sharing the constant between the two commands.

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.

2 participants