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

chore: Reuse logic from common functions #402

Closed
wants to merge 1 commit into from

Conversation

nshenry03
Copy link
Contributor

From @MaxymVlasov in #401 (comment):

Heh, you found code written in Paleolithic.

Let's try reuse logic that used in common functions here:

https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/_common.sh#L178

Absolutely have no idea why it was written in so complex way

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Just doing as @MaxymVlasov asked in #401 (comment)

How can we test changes

None. It's a fairly simple and straightforward change.

From @MaxymVlasov in antonbabenko#401 (comment):
> Heh, you found code written in Paleolithic.
>
> Let's try reuse logic that used in common functions here:
>
> https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/_common.sh#L178
>
> Absolutely have no idea why it was written in so complex way
@nshenry03 nshenry03 changed the title refactor: Reuse logic from common functions chore: Reuse logic from common functions Jun 30, 2022
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

If this is exactly what @MaxymVlasov meant in #401 (comment) (as opposite to re-working this hook to use common::per_dir_hook common func), then LGTM

@MaxymVlasov
Copy link
Collaborator

Hmm, literally, maybe it can be rewriten to common::per_dir_hook 🤔
But basically, I mean exactly what done in this PR.

@nshenry03
Copy link
Contributor Author

Sorry, @MaxymVlasov , maybe I don't understand... Do you want me to call the common::per_dir_hook function somehow from the terraform_validate_ function? Or does this MPR look okay for approval?

Just seeing if you need anything else from me. Thanks!

@MaxymVlasov
Copy link
Collaborator

Close in #404

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 this pull request may close these issues.

3 participants