Skip to content

[profile] Migrate profiling middleware to orchard.profile #929

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

Merged
merged 2 commits into from
Apr 9, 2025

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Apr 8, 2025

Sister PR: clojure-emacs/orchard#333

  • Bumped Orchard
  • Removed thunknyc/profile from dependencies
  • Dropped several ops from the middleware – things like individual function summaries, tuning max sample count, stuff like that. Only four ops remain: toggle profile for a var, for a whole ns, clear results, and render results via the inspector.
  • Renamed the remaining ops to have cider/ prefix. I briefly tried keeping the semblance of backward compatibility and keep old names too, but since the most important op (summary) is not compatible with its old implementation, it provides no value. GIven that the feature is an obscure one, I don't care much about breakages here.
  • Added :orchard.profile/profiled as a relevant meta key – we can then outline vars with it in the same way we do with traced vars.

  • You've added tests to cover your change(s)
  • You've updated the README
  • Middleware documentation is up to date

Copy link
Member

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

Renamed the remaining ops to have cider/ prefix. I briefly tried keeping the semblance of backward compatibility and keep old names too, but since the most important op (summary) is not compatible with its old implementation, it provides no value. GIven that the feature is an obscure one, I don't care much about breakages here.

Agreed. I'm pretty sure that was used by CIDER only anyways.

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Apr 8, 2025

@bbatsov Are you OK with the cider/ prefix? I also see there is cider.clj-reload/*, so I wonder what's the final idea for consistent prefixes.

@bbatsov
Copy link
Member

bbatsov commented Apr 8, 2025

@bbatsov Are you OK with the cider/ prefix? I also see there is cider.clj-reload/*, so I wonder what's the final idea for consistent prefixes.

Yeah, the plan is to prefix everything with "cider/". This is the way!

@bbatsov
Copy link
Member

bbatsov commented Apr 9, 2025

Here are a bit more details about my plan for adopting a consistent cider/ op prefix #710 Not a lot of work overall, but somehow I never got to doing it.

@alexander-yakushev alexander-yakushev merged commit fbabdeb into master Apr 9, 2025
16 checks passed
@alexander-yakushev alexander-yakushev deleted the profile branch April 9, 2025 06:20
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