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

fix: Show generator description from external plopfiles #402

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

Conversation

rosen-kanev
Copy link

When loading generators from external plop files with plop.load(path) their description was being lost. The root cause is that the proxy object that we're passing to setGenerator() doesn't have a description property, which judging by the rest of the code is by design.

I did contemplate an alternative fix - to add the description in the setGenerator function like so:

// add the generator to this context
generators[name] = Object.assign(
  {},
  config.proxy ? { description: config.proxy.getGenerator(name) } : {},
  config,
  {
    name: name,
    basePath: plopfilePath,
  },
);

but IMO the patch should be where the external plop file is loaded.

This PR fixes the missing description logged in #247.


Aside: @piersolenski observed that setWelcomeMessage() also is being lost, but I don't think it's a good design to allow the loaded files to change it.

A generator's description wasn't set properly when the generator was in
an external plop file loaded with `plop.load("./another-plopfile.js")`.

Fixes plopjs#247
@crutchcorn
Copy link
Member

Potentially weird Q: Is there overlap with this PR and this one? #394

@rosen-kanev
Copy link
Author

When I stumbled upon the missing description I looked around the existing issues and PRs and did try #394 locally. The description is missing there too (though it would've been a very elegant bugfix if it was just a shallow copy 😅 )

@tobiashochguertel
Copy link

The fix in the commit #b89944ea8c4e9709c33491a87f80c81f7d378aeb works so far. Would it be possible to get this quick merged and released?

Can I do something?

@rosen-kanev
Copy link
Author

@crutchcorn I don't like generating extra noise unless necessary, but in case you have your notifications turned on only for mentions.

@kemalsecer
Copy link

Are there plans to merge this fix in the near future?

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

4 participants