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

feat: support configurable contants for predicates #998

Merged

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf added the feat Issue is a feature label May 11, 2023
@Torres-ssf Torres-ssf self-assigned this May 11, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
95.37% (+0.04% 🔼)
5255/5510
🟢 Branches
84.51% (+0.19% 🔼)
862/1020
🟢 Functions
87.09% (+0.04% 🔼)
958/1100
🟢 Lines
95.37% (+0.04% 🔼)
5026/5270

Test suite run success

896 tests passing in 140 suites.

Report generated by 🧪jest coverage report action from 5698280

@Torres-ssf

This comment was marked as resolved.

@Torres-ssf Torres-ssf marked this pull request as ready for review May 12, 2023 02:13
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Great stuff! 🚀

The docs say that configurables can be a "flexible mechanism for customizing Predicates behavior", but in my understanding, we're deriving (factoring) new Predicates instead. No?

In other words, if the use of configurables recomputes the Predicate's bytes and address and results in a new Predicate entirely, the first sentence might give a wrong idea.

I wonder if my assumptions are all correct. If they are, these details must be better explained in the docs.

Since much of the configurables magic is done internally, it can be challenging for users to grasp it unless it is explicitly mentioned.

packages/predicate/src/predicate.ts Outdated Show resolved Hide resolved
@Torres-ssf
Copy link
Contributor Author

Great stuff! rocket

The docs say that configurables can be a "flexible mechanism for customizing Predicates behavior", but in my understanding, we're deriving (factoring) new Predicates instead. No?

In other words, if the use of configurables recomputes the Predicate's bytes and address and results in a new Predicate entirely, the first sentence might give a wrong idea.

I wonder if my assumptions are all correct. If they are, these details must be better explained in the docs.

Since much of the configurables magic is done internally, it can be challenging for users to grasp it unless it is explicitly mentioned.

@arboleya Yeah, I agree with you. I've added a more detailed explanation.

@Torres-ssf Torres-ssf requested a review from arboleya May 14, 2023 20:04
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Minor tyop.

packages/predicate/src/predicate.ts Outdated Show resolved Hide resolved
arboleya and others added 2 commits May 15, 2023 06:05
@Torres-ssf Torres-ssf requested a review from arboleya May 15, 2023 11:09
@SilentCicero
Copy link
Member

Great work here. This will be very useful for all our predicate infrastructure.

@Torres-ssf Torres-ssf merged commit 7c8439f into master May 15, 2023
@Torres-ssf Torres-ssf deleted the st/feat/support-configurable-contants-for-predicates branch May 15, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable Constants For Predicates
4 participants