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

Should we extract StashUnstagedChanges#initial_commit? to a module or make it public? #728

Closed
Quintasan opened this issue Sep 22, 2020 · 3 comments · Fixed by #729
Closed
Labels

Comments

@Quintasan
Copy link
Contributor

~/overcommit/lib/overcommit/hook/pre_commit/execute_permissions.rb:29: warning: Overcommit::Hook::PreCommit::Base#initial_commit? at ~/.rbenv/versions
/2.6.5/lib/ruby/2.6.0/forwardable.rb:158 forwarding to private method Overcommit::HookContext::PreCommit#initial_commit?

Originally posted by @sds in #727 (comment)

@Quintasan
Copy link
Contributor Author

@sds We can either:

  1. Make StashUnstashedChanges#initial_commit? public.
  2. Extract it into it's own module and include it where needed.

As you are the maintainer I'm going to defer to your call on this but my personal opinion is that we should extract it to a module on it's own as it's not exclusively related to stashing and unstashing changes.

@Quintasan Quintasan changed the title initial_commit? forwarded to private method in StashUnstagedChanges Should we extract StashUnstagedChanges#initial_commit? to a module or make it public? Sep 22, 2020
@Quintasan
Copy link
Contributor Author

@sds I went with the smallest change possible seeing as you didn't respond. We can always extract this particular function to a module later on if really necessary.

@sds sds closed this as completed in #729 Sep 25, 2020
sds pushed a commit that referenced this issue Sep 25, 2020
@sds
Copy link
Owner

sds commented Sep 25, 2020

Sorry I missed your suggestion, @Quintasan. Fine with the current approach. Thank you.

@sds sds added the question label Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants