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

(PDOC-252) Add describe features to puppet-strings face #183

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

kris-bosland
Copy link
Contributor

Hey @eputnam , here is that PR that I was talking about. LMK when you have had some time to look at this, and then I would like to talk about how I can pull in the data I need to finish describe features. Thanks!

@kris-bosland kris-bosland force-pushed the fix/master/pdoc-252 branch 5 times, most recently from 66d8871 to caf4eed Compare June 14, 2018 21:46
Copy link
Contributor

@eputnam eputnam left a comment

Choose a reason for hiding this comment

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

Looks pretty good from what I can see. I'd be interested in meeting up though to go over it further and figure out what else we need and what's next.

render_json(file)
end

# If outputting Markdown, render the output
if options[:markdown]
render_markdown(file)
end

if options[:describe]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to have --describe as a generate option, we'll need to register it in the face https://github.com/puppetlabs/puppet-strings/blob/master/lib/puppet/face/strings.rb#L7

on further examination this is just something that the action passes to generate, i guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am just using it to carve out a space in the generate options, since I expect most of the describe options to not be applicable to the other modes. Probably a little awkward style wise.

types/**/*.pp
lib/**/*.rb
)
PuppetStrings::generate(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about trying to create a PuppetStrings::describe method separate from PuppetStrings::generate. We may be able to cut some of the cruft out to reduce load time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions for stuff we don't need for describe @eputnam ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I forget now what I was thinking here. I guess that describe doesn't need to look at everything we list in generate normally? But as for what that is, I can't remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess the search_patterns I set up here are much less than the regular generate. I think for now I will focus on filling in missing data, then I will come back and look for performance improvements.

# @param [bool] list Create the summarized list instead of describing each type.
# @param [bool] providers Show details of the providers.
# @return [void]
def self.describe(describe_types = [], list = false, providers = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

sort of a nit, but we've been calling the methods that create the final output render

@kris-bosland kris-bosland changed the title (PDOC-252) Add describe features to puppet-strings face (WIP) (PDOC-252) Add describe features to puppet-strings face Nov 1, 2018
@kris-bosland
Copy link
Contributor Author

Hey @eputnam , I think this is ready for merge - it isn't a complete reimplementation of describe, but I think it makes more sense to do this work in several pull requests. This does pull in the missing pieces of the file type that we were looking at back in June.

@eputnam eputnam merged commit 2987558 into puppetlabs:master Nov 6, 2018
@kris-bosland kris-bosland deleted the fix/master/pdoc-252 branch February 12, 2019 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants