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

Added parameter checking. #4

Closed
wants to merge 1 commit into from
Closed

Conversation

manwar
Copy link

@manwar manwar commented Jan 22, 2021

Hi @ferki

I received the distribution as my January 2021 assignment for Pull Request Club.
Please review my proposed change introducing the parameter checks.
I would be more than happy if you approve and merge the change.

Many Thanks.

Best Regards,
Mohammad

Copy link
Owner

@ferki ferki left a comment

Choose a reason for hiding this comment

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

Thanks, @manwar, for your contribution, for your work around Perl, and for your patience here!

I have to admit I've spent quite some time thinking about how to approach this PR, even if that work was not visible here so far. I'd like to summarize my thoughts now.

  1. This PR doesn't pass the automated test suite in its current form.

  2. It's not entirely clear to me what real world problem are we trying to solve here exactly. We had no chance to discuss the context and potential solutions beforehand. The only perceived or explicit reason for this PR is the Pull Request Challenge.

  3. Not sure how much does it worth exactly to check for parameters here, and I feel there is already more effort invested than saved. The intended usage for this hook is from a Rex::Commands::File::file() call, which fails much earlier if undef is passed as the file to operate on. Perhaps it's better to highlight the intended usage of this subroutine by marking it (and others) as private. But this is exactly the discussion I would rather make on an issue before jumping to any specific implementation.

Combining all the above, I have decided not to merge this PR. Instead, I've created a Contributing guide for all my repositories in order to ensure a better experience for all participants next time.

Of course, I would be happy to collaborate with you on this or other topics in the future. Please feel free to (re)open issues and (re)submit changes as needed.

Rest assured, it is still an accepted PRC assignment for you, and you definitely triggered valuable progress for the benefit of all, and I'm grateful for that ❤️

@ferki ferki closed this Jun 23, 2021
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