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

Rename org-gtd-review.el to org-gtd-reflect.el #168

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

doolio
Copy link
Contributor

@doolio doolio commented May 24, 2023

This renaming aligns it with the name for step 4 of the GTD methodology. All commands renamed accordingly.

Resolves: #164

@doolio
Copy link
Contributor Author

doolio commented May 24, 2023

I believe the order of commands/functions is how you expect to see them so no changes required in that regard.

changes-for-3.0.org Outdated Show resolved Hide resolved
This renaming aligns it with the name for step 4 of the GTD
methodology.  All commands renamed accordingly.

Resolves: Trevoke#164
@Trevoke
Copy link
Owner

Trevoke commented May 24, 2023

There are conflicts because of the latest bugfix I merged in (which gitignores the autoloads file, among other things).

Here's the fixes that I think we need to apply / open questions left to answer:

  • adjust CHANGELOG to match what's on the main branch now
  • remove autoloads file
  • please use version numbers and not dates in the make-obsolete statements (e.g. "3.1.0", since that's the next version release)
  • you don't need to write this much for the second argument of make-obsolete since you can just put the symbol for the new function name (at least I'm 90% sure that (make-obsolete 'org-gtd-review-stuck-calendar-items 'org-gtd-reflect-stuck-calendar-items "3.1.0") will work as needed, but I could be wrong)
  • Is this the right way to set up the autoloads? Shouldn't the functions have autoload cookies as well? Did you test this to make sure both old and new functions properly loaded and executed without a require statement?
  • make sure you are only changing NEW documentation, nothing in the past releases (this means "changes for 3.0" can't be touched, and none of the "upgrade path" sections can be touched either)
  • the test/reviews-test.el should also be changed to test/reflect-test.el

@doolio
Copy link
Contributor Author

doolio commented May 24, 2023

please use version numbers and not dates in the make-obsolete statements (e.g. "3.1.0", since that's the next version release)

OK.

you don't need to write this much for the second argument of make-obsolete since you can just put the symbol for the new function name (at least I'm 90% sure that (make-obsolete 'org-gtd-review-stuck-calendar-items 'org-gtd-reflect-stuck-calendar-items "3.1.0") will work as needed, but I could be wrong)

OK, I simply followed the example suggestion in the make-obsolete docstring as it also updates the related docstring. I'll look to see what you suggest does to the docstring if anything.

Is this the right way to set up the autoloads?

I included an autoload cookie as the example in the docstring included one I believe or I saw it being included in other org files when a command is being made obsolete.

Shouldn't the functions have autoload cookies as well?

Well, yes (and I had those in mind in raising #156 but as they didn't originally I didn't give them one in this PR.

Did you test this to make sure both old and new functions properly loaded and executed without a require statement?

No, but I will now that you have removed the autoloads file.

make sure you are only changing NEW documentation, nothing in the past releases (this means "changes for 3.0" can't be touched, and none of the "upgrade path" sections can be touched either)

OK, I'll undo what I have done in those sections.

the test/reviews-test.el should also be changed to test/reflect-test.el

OK, thanks.

@Trevoke
Copy link
Owner

Trevoke commented May 25, 2023

This looks good, thank you! Tell me about the autoloads? What did you learn?

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.

org-gtd-review.el should be renamed org-gtd-reflect.el
2 participants