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

Improve the targets in doc/Makefile #1014

Merged
merged 20 commits into from
Mar 8, 2021

Conversation

core-man
Copy link
Member

@core-man core-man commented Mar 7, 2021

Description of proposed changes

  • help target
    • List information of clean, api, and serve
    • Tiny improvement for the html information
  • Change the order of all the targets
    • the original order: all, help, clean, html, api, linkcheck, doctest, serve
    • the new order: help, all, html, api, serve, linkcheck, dotest, clean.
  • Tiny improvment of the clean target

The following are some of my thoughts on which I want your comments:
- [ ] Now the all target is the same as the html, so shall we remove the all target?

Fixes #914

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@core-man core-man requested a review from seisman March 7, 2021 10:05
@seisman seisman added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Mar 8, 2021
@seisman seisman added this to the 0.3.1 milestone Mar 8, 2021
@seisman
Copy link
Member

seisman commented Mar 8, 2021

  • Change the order of all the targets

    • the original order: all, help, clean, html, api, linkcheck, doctest, serve
    • the new order: help, all, html, api, serve, linkcheck, dotest, clean.

Putting help in the first place makes more sense to me. In the old Makefile, running make builds the documentation. Now running make shows the help message.

The following are some of my thoughts on which I want your comments:

  • Now the all target is the same as the html, so shall we remove the all target?

I still want to keep the all target, because it's commonly used in almost all Makefiles.

@weiji14 is removing both linkcheck and doctest targets in PR #991. I think it makes more sense to remove it in this PR. @weiji14 thoughts?

  • There are modules and ipynb_checkpoints in the clean target, but I cannot find them after I made all the targets. So remove them?

Sounds OK to me.

  • Shall we rename the serve target to be server?

I have seen both in other projects. So I'm OK with either.

@weiji14
Copy link
Member

weiji14 commented Mar 8, 2021

@weiji14 is removing both linkcheck and doctest targets in PR #991. I think it makes more sense to remove it in this PR. @weiji14 thoughts?

Yes please do it here instead, feel free to cherry-pick commit 89f1f94 and f04173b.

@core-man
Copy link
Member Author

core-man commented Mar 8, 2021

@weiji14 is removing both linkcheck and doctest targets in PR #991. I think it makes more sense to remove it in this PR. @weiji14 thoughts?

Yes please do it here instead, feel free to cherry-pick commit 89f1f94 and f04173b.

hmm~ I even did not know cherry-pick. Now I understand why we need to submit each small revision in one commit.
Done in 290bb2a and 5cf3674.

@core-man
Copy link
Member Author

core-man commented Mar 8, 2021

  • There are modules and ipynb_checkpoints in the clean target, but I cannot find them after I made all the targets. So remove them?

Sounds OK to me.

Done in 61ff7b7.

  • Shall we rename the serve target to be server?

I have seen both in other projects. So I'm OK with either.

Done in 4733351.

doc/Makefile Outdated Show resolved Hide resolved
doc/Makefile Outdated Show resolved Hide resolved
doc/Makefile Outdated Show resolved Hide resolved
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Looking good, just one more suggestion on alphabetically ordering the Makefile targets.

Also, could you change the title of this PR to be a bit more descriptive? E.g. "Improve help section of Makefile" or something like that. It's generally a good idea to be more specific on what are the main changes that were made so that people scanning the git log know what happened.

doc/Makefile Outdated
Comment on lines 17 to 20
@echo " html build the HTML files from the existing rst sources"
@echo " api generate rst source files of API documentation"
@echo " server make a local HTTP server for previewing the built documentation"
@echo " clean clean up built and generated files"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe sort these lines alphabetically?

Suggested change
@echo " html build the HTML files from the existing rst sources"
@echo " api generate rst source files of API documentation"
@echo " server make a local HTTP server for previewing the built documentation"
@echo " clean clean up built and generated files"
@echo " api generate rst source files of API documentation"
@echo " clean clean up built and generated files"
@echo " html build the HTML files from the existing rst sources"
@echo " server make a local HTTP server for previewing the built documentation"

I'd also prefer it if the rest of this Makefile was sorted alphabetically (i.e. all, api, clean, html, server), while keeping this help target on top of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

People usually put clean to be the last one, while the target like all or install/build/html is the first one in a makefile. Shall we keep this rule? @seisman @willschlitzer How do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to put clean at the bottom.

Copy link
Member Author

Choose a reason for hiding this comment

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

@weiji14 @seisman Change the order to be all, api, html, server, and clean in e719f74. So keep all first, clean last, and sort the other alphabetically.

@core-man core-man changed the title Improve doc/makefile Improve the help target of doc/Makefile Mar 8, 2021
@core-man
Copy link
Member Author

core-man commented Mar 8, 2021

Also, could you change the title of this PR to be a bit more descriptive? E.g. "Improve help section of Makefile" or something like that. It's generally a good idea to be more specific on what are the main changes that were made so that people scanning the git log know what happened.

Great~ I once thought to always have a title that can represent all the changes in one PR:joy:. Change to be "Improve the help target of doc/Makefile"

doc/Makefile Outdated Show resolved Hide resolved
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
doc/Makefile Outdated Show resolved Hide resolved
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
doc/Makefile Outdated Show resolved Hide resolved
core-man and others added 4 commits March 8, 2021 22:53
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good to me, but ping @weiji14 @meghanrjones @willschlitzer for comments.

@seisman
Copy link
Member

seisman commented Mar 8, 2021

@core-man Please update the PR description based on the changes in this PR.

@core-man core-man changed the title Improve the help target of doc/Makefile Remove linkcheck and doctest and improve all the other targets in doc/Makefile Mar 8, 2021
@core-man core-man changed the title Remove linkcheck and doctest and improve all the other targets in doc/Makefile Remove linkcheck/doctest and improve other targets in doc/Makefile Mar 8, 2021
doc/Makefile Outdated Show resolved Hide resolved
doc/Makefile Outdated Show resolved Hide resolved
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Nice!

doc/Makefile Outdated Show resolved Hide resolved
@seisman seisman changed the title Remove linkcheck/doctest and improve other targets in doc/Makefile Improve the targets in doc/Makefile Mar 8, 2021
@seisman seisman merged commit 5385fa5 into GenericMappingTools:master Mar 8, 2021
@core-man core-man deleted the doc-makefile branch March 14, 2021 09:08
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
- Remove useless "linkcheck" and "doctest" targets
- Improve the "clean" target
- Improve the verbose messages of "api" and "server" targets
- Rename the "serve" target to "server"

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Meghan Jones <meghanj@alum.mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more targets to doc/makefile help information
4 participants