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

Add ability to generate REFERENCE.md and test it can be generated #530

Closed

Conversation

ghoneycutt
Copy link
Member

@@ -41,6 +41,11 @@ RSpec::Core::RakeTask.new(:acceptance) do |t|
t.pattern = 'spec/acceptance'
end

desc 'Generate REFERENCE.md'
task :reference do
sh 'puppet strings generate --format markdown'
Copy link
Member

Choose a reason for hiding this comment

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

should we call this via bundle exec? I don't think that everybody has puppet installed in $PATH at their workstations. At least I don't.

Copy link
Member

Choose a reason for hiding this comment

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

I assume that you'd execute bundle exec rake reference then and it'd work that way.

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this configurable? In https://github.com/voxpupuli/puppet-extlib#development it's slightly different.

Copy link
Member

Choose a reason for hiding this comment

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

Ah good catch. We also used this in the past on other modules:

bundle exec rake strings:generate\[',,,,false,true']

Copy link
Member Author

Choose a reason for hiding this comment

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

what needs to be configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get why it needs customizable options.

Copy link
Member

Choose a reason for hiding this comment

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

To quote the earlier linked extlib README:

Reference documentation is generated using puppet-strings. To regenerate it, please run the rake task as follows.

 % bundle exec rake strings:generate\['lib/puppet/functions/extlib/*.rb,,,,false,true']

This is done to avoid generating docs for our deprecated functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha.. though if the functions still exist, they should have documentation.

Copy link
Member

Choose a reason for hiding this comment

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

To be explicit, I was thinking about sh 'puppet strings generate --format markdown<%= strings_options %>' or something like that. Then extlib can add arguments in .sync.yml and we'll have a single command will just work in all modules even if the implementation is slightly different.

I'm also thinking about contributing strings:generate to https://github.com/puppetlabs/puppet-strings instead of solving it in our modulesync.

puppetlabs/puppetlabs_spec_helper#274 (added benefit that PDK users will also get this)

Copy link
Member

Choose a reason for hiding this comment

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

puppetlabs/puppet-strings#192 is what I actually meant. It doesn't solve the patterns issue though.

@ghoneycutt ghoneycutt force-pushed the generate_reference_docs branch from 2c7fdc5 to 8689be6 Compare January 3, 2019 23:10
@ghoneycutt
Copy link
Member Author

@bastelfreak anything left here?

@@ -116,6 +116,7 @@ Rakefile:
default_enabled_rake_targets:
- 'metadata_lint'
- 'release_checks'
- 'reference'
Copy link
Member

Choose a reason for hiding this comment

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

Not all modules generate meaningful references. I'm not sure we should add this by default just yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not hurt to generate it even if they don't have great output. It does hurt the project by not conforming to the Forge standards and creating the REFERENCE.md output. I think we should generate them, even if they are not 100% awesome and then work toward improving them.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could use a way to generate some overview of all our modules to get some insight. https://github.com/voxpupuli/puppet-lint-param-docs can check but voxpupuli/puppet-lint-param-docs#11 would be an improvement. Also some tools to convert from the old docs format would greatly improve things.

Another issue I have is that I'm not a fan of including generated files in git because it's so much noise in git log and a great way to get merge conflicts.

Copy link
Member Author

Choose a reason for hiding this comment

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

The rake task just shows that the docs can be generated. It is not meant to be run with each pull request. The actual generation of docs that is committed to git would be done as part of the release process.

+1 to using https://github.com/voxpupuli/puppet-lint-param-docs though that's a separate issue as it forces compliance.

This PR is a step toward that, without having to force compliance with that linting. If you have docs, you should get the generated file.

There is no need to convert old docs as the REFERENCE.md file would be new and does not conflict with README.md. If a README lists all the params, their values, etc, we should remove that in favor of using REFERENCE.md, though the two are not tightly coupled.

First we should implement the ability to generate REFERENCE.md, then we should update README's and document code, then enforce that with the linting to ensure the quality continues.

Copy link
Member

Choose a reason for hiding this comment

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

The rake task just shows that the docs can be generated. It is not meant to be run with each pull request. The actual generation of docs that is committed to git would be done as part of the release process.

That's a misinterpretation. This config value means to run these checks when running rake test. IMHO generating the REFERENCE.md when testing isn't the right thing to do.

There is no need to convert old docs as the REFERENCE.md file would be new and does not conflict with README.md. If a README lists all the params, their values, etc, we should remove that in favor of using REFERENCE.md, though the two are not tightly coupled.

I didn't mean that way of old docs (separate issue). A lot of our docs still use the old format (puppet-doc?) instead of yardoc. puppet-strings doesn't know how to deal with that and generates garbage. We can probably come up with an automatic converter to save a lot of effort.

First we should implement the ability to generate REFERENCE.md, then we should update README's and document code, then enforce that with the linting to ensure the quality continues.

Right, but just adding the task to Rakefile.erb is sufficient for that. This config line is't needed.

@ghoneycutt
Copy link
Member Author

Not sure how to make it configurable. Suggest opening a new PR based on this and adding your changes there.

@ekohl
Copy link
Member

ekohl commented Jan 11, 2019

Not sure how to make it configurable. Suggest opening a new PR based on this and adding your changes there.

See #543

@ekohl
Copy link
Member

ekohl commented Apr 6, 2019

Closing in favor of #543

@ekohl ekohl closed this Apr 6, 2019
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.

3 participants