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

Man page #158

Merged
merged 4 commits into from
Nov 18, 2017
Merged

Man page #158

merged 4 commits into from
Nov 18, 2017

Conversation

avdv
Copy link
Collaborator

@avdv avdv commented Nov 10, 2017

Description

This PR adds a manual page for colorls. It tries to automatically make it work on the user's system by using the manpages gem.

The man page is written in ronn format.

In the future, the man page could probably be semi-generated using the defined options in the code...

  • Relevant Issues : Support for man page #80
  • Relevant PRs : (none)
  • Type of change :
    • New feature
    • Bug fix for existing feature
    • Code quality improvement
    • Addition or Improvement of tests
    • Addition or Improvement of documentation

@athityakumar
Copy link
Owner

@avdv - Is there a way we can automatically generate the man page by passing the opts variable formed by Optparse module, to manpages?

@athityakumar
Copy link
Owner

Also, the formed man file from rake man has to be put in the proper system directory so that man colorls is picked up. For example, cp man/colorls.1 /usr/local/share/man/man1/colorls.1 works for Mac.

@avdv
Copy link
Collaborator Author

avdv commented Nov 11, 2017

Also, the formed man file from rake man has to be put in the proper system directory so that man colorls is picked up. For example, cp man/colorls.1 /usr/local/share/man/man1/colorls.1 works for Mac.

This is handled by the manpages gem:

This plugin automatically hooks into the ruby gems system. Every gem installed afterwards is checked for manpages. If this gem finds them, it exposes them to the man command.

I can confirm that it works for Linux. They say it works for Mac, too. Just try it!

If you wanted to install it to the system, you would need to run this as root... Also, throughout the net, consensus seemed to be that this would be the job of a package manager, like rpm or homebrew.

@avdv
Copy link
Collaborator Author

avdv commented Nov 11, 2017

@avdv - Is there a way we can automatically generate the man page by passing the opts variable formed by Optparse module, to manpages?

Yes, I am looking into it...

@avdv
Copy link
Collaborator Author

avdv commented Nov 12, 2017

I have updated this branch, the options of the man page are now generated.

@avdv avdv force-pushed the man-page branch 2 times, most recently from 998dee3 to 8216394 Compare November 13, 2017 20:47
@avdv
Copy link
Collaborator Author

avdv commented Nov 13, 2017

Another update... alas, the travis build still fails.

The problem is the rubocop violation of:

colorls.gemspec:40:24: C: Style/GlobalVars: Do not introduce global variables.
  spec.files         = $files
                       ^^^^^^

I had introduced the global method, since otherwise rubocop complained about the number of lines in the block:

colorls.gemspec:24:1: C: Metrics/BlockLength: Block has too many lines. [28/25]
Gem::Specification.new do |spec| ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What are the options here? Exclude the gemspec file? Increase the limit?

@athityakumar
Copy link
Owner

Disable the Metrics/BlockLength cop before the block, with # rubocop:disable Metrics/BlockLength

@avdv
Copy link
Collaborator Author

avdv commented Nov 13, 2017

Thanks! Done.

@avdv avdv force-pushed the man-page branch 2 times, most recently from 35edf61 to 1692c79 Compare November 13, 2017 21:49
@athityakumar
Copy link
Owner

Can the man/colorls.1 be gitignored?

Rakefile Outdated
task default: %w[spec rubocop]

task install: 'man/colorls.1'
Copy link
Owner

Choose a reason for hiding this comment

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

Works well. The formed man file is automatically exposed to manpath - cool! Confirmed for Mac.

One question though - if there's already a man/colorls.1 file, rake installing doesn't re-form the man file. So, any changes in the ronn file doesn't get reflected unless the existing man/colorls.1 is manually removed? Can a File.delete('man/colorls.1') be included within the task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can the man/colorls.1 be gitignored?

Yes, good point!

I'll look into the rake task issue, probably need to define a phony target...

@avdv
Copy link
Collaborator Author

avdv commented Nov 14, 2017

I have updated the branch according to your comments:

  • added man/colorls.1 to the .gitignore file
  • refined the man/colorls.1 Rake task to depend on the source files
  • removed the explicit dependency of the install task on the man/colorls.1 task, since Rake does that automatically, due to the gemspec files

@athityakumar
Copy link
Owner

@avdv - Awesome work. Just add "Man pages have been added. Checkout man colorls" to both README and POST_INSTALL_MESSAGE of gemspec file. We're almost ready to merge. 👍


spec.add_development_dependency 'bundler', '~> 1.15'
spec.add_development_dependency 'diffy'
spec.add_development_dependency 'rake'
spec.add_development_dependency 'ronn'
Copy link
Owner

Choose a reason for hiding this comment

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

Should ronn rather be a runtime dependency? When a user does gem install colorls, is the manpages created by default by ronn? For that, we'd require ronn to be installed while installing colorls right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the man page is generated when building the package/gem, ie. when running rake.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, so how does the user generate the man page? The user is going to only do gem install. Not git clone and rake install. Should we have a --setup flag for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The manpage is part of the gem, it is included. This way it is only generated once and then uploaded to the gem server.

Copy link
Owner

Choose a reason for hiding this comment

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

If it's part of the gem, should it NOT be git-ignored? I thought that the user would be generating the man pages after installing colorls, that's why I asked to gitignore colorls.1 file. But if we're to generate the man file and push to Rubygems too, it shouldn't be gitignored.

@avdv
Copy link
Collaborator Author

avdv commented Nov 15, 2017

I have adjusted the message and README as per your comment. :-)

@athityakumar
Copy link
Owner

One final query: If the man file is a part of the gem, it should NOT be gitignored right? I thought that the user would be generating the man pages after installing colorls, that's why I asked to gitignore colorls.1 file. But if we're to generate the man file and push to Rubygems too, it shouldn't be gitignored.

@avdv
Copy link
Collaborator Author

avdv commented Nov 17, 2017

One final query: If the man file is a part of the gem, it should NOT be gitignored right? I thought that the user would be generating the man pages after installing colorls, that's why I asked to gitignore colorls.1 file. But if we're to generate the man file and push to Rubygems too, it shouldn't be gitignored.

I guess that depends on your policy. Usually my policy is to never check in something that is generated. Thusly, it should be added to git ignore.

But I also have seen projects using ronn, that commit the generated files. Actually, I do not see the benefit of it... except that you would not have to add the files to the gemspec manually.

So, OK. I'll change that.

When this gem is installed, it automatically symlinks man pages of other gems
so that you can view them with `man`.
@avdv
Copy link
Collaborator Author

avdv commented Nov 17, 2017

@athityakumar OK, done!

@athityakumar athityakumar merged commit b122811 into athityakumar:master Nov 18, 2017
@athityakumar athityakumar mentioned this pull request Nov 18, 2017
@avdv avdv deleted the man-page branch November 18, 2017 13:17
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