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

Edit pattern.md #176

Merged
merged 2 commits into from
Feb 19, 2024
Merged

Edit pattern.md #176

merged 2 commits into from
Feb 19, 2024

Conversation

jariji
Copy link
Contributor

@jariji jariji commented Feb 9, 2024

I made some edits to pattern.md. I filed some issues while I was going.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a6891f8) 61.18% compared to head (43e5cb9) 61.52%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   61.18%   61.52%   +0.33%     
==========================================
  Files          28       28              
  Lines        1636     1627       -9     
==========================================
  Hits         1001     1001              
+ Misses        635      626       -9     

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

@thautwarm
Copy link
Owner

Will review this PR and #166 today.

Copy link
Owner

@thautwarm thautwarm left a comment

Choose a reason for hiding this comment

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

Things look fine to me.

It seems you discard this:

However, remember, due to some Julia optimization details, even if the guards can be put in the middle of a matching process, it is still better to postpone it until the end of matching sequence. This allows for better performance.

It's up to you whether to add the statement back. I haven't made any benchmark for the performance diff for a long time, but I do know a few cases that MLStyle's optimizations become meaningless in higher Julia version.

@thautwarm
Copy link
Owner

@jariji If you are ready for this PR to get merged, just mention me again.

@jariji
Copy link
Contributor Author

jariji commented Feb 18, 2024

@thautwarm I'm ready for this to be merged.

It's up to you whether to add the statement back. I haven't made any benchmark for the performance diff for a long time, but I do know a few cases that MLStyle's optimizations become meaningless in higher Julia version.

I felt that the ordering optimization was too minor an issue to be raised in introductory docs.

@thautwarm thautwarm merged commit 9c9eb00 into thautwarm:main Feb 19, 2024
17 checks passed
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