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

Make the rake task depend only on rake #1457

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

Conversation

elia
Copy link

@elia elia commented Sep 1, 2022

Description

Without this it's necessary to require the whole library just to have
the task defined. With this change yard will be required before
executing the task code reducing the load time and memory footprint
of libraries defining the task.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

Without this it's necessary to require the whole library just to have
the task defined. With this change `yard` will be required before
executing the task code reducing the load time and memory footprint
of libraries defining the task.
@elia elia marked this pull request as ready for review September 1, 2022 12:57
elia added a commit to nebulab/solidus that referenced this pull request Sep 1, 2022
Copy link
Owner

@lsegal lsegal left a comment

Choose a reason for hiding this comment

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

I'm generally worried that this advice may break plugins that rely on YARD's plugin loading features.

YARD relies heavily on autoload, and thus generally loads fairly lazily (minus the above plugin caveat), so generally speaking, the intended and recommended way to require YARD is to do so via require 'yard', pulling in any specific classes via autoload. The location of specific Ruby sources within the YARD package are generally not considered part of our "public API", and thus, requiring the YardocTask directly in this way could break if we ever moved the file.

Is there a reason why you think this is needed? Is there a specific performance issue that necessitates this change?

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.

None yet

2 participants