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

Types #16

Merged
merged 16 commits into from
Jan 14, 2024
Merged

Types #16

merged 16 commits into from
Jan 14, 2024

Conversation

HPWebdeveloper
Copy link
Owner

@HPWebdeveloper HPWebdeveloper commented Jan 13, 2024

@HPWebdeveloper
Copy link
Owner Author

HPWebdeveloper commented Jan 13, 2024

FYI, @3m1n3nc3 @SSEsmaeeli


@SSEsmaeeli you can send a new PR to the branch (types) to conclude this PR (Types#16)?

This time please:

  • Check for any other missing types including those in your recent PR
  • Don't remove any PHPDocs like it was removed in that PR, They should be kept for additional information, and they are also automatically sanitized by the GitHub flow.

@HPWebdeveloper HPWebdeveloper merged commit d661d59 into main Jan 14, 2024
16 checks passed
@HPWebdeveloper HPWebdeveloper deleted the types branch January 14, 2024 11:25
@3m1n3nc3
Copy link
Contributor

Removing the optional parameters for the log reference generator introduces another small problem, for instance, I use the Valorin\Random\Random::string() to generate log reference, and this allows me to pass parameters that determines the length of the string, if I need uppercase, lowercase, numbers or symbols in the generated string. Now I am limited to only providing the lenght of the string.

@SSEsmaeeli
Copy link
Contributor

SSEsmaeeli commented Jan 14, 2024

There are a couple of considerations here:

  • This package is not a generator package, so a simple generator class would be enough to handle the token generation. If someone needs more details, he or she can implement his or her own thoughts and be able to achieve what you said.

  • The previous code won't give more chances to the person who is using the package. The only parameter is length, which is considered inside the code as $refGen[2].

     `$refGen = config('pay-pocket.log_reference_generator', [
          Str::class, 'random', [config('pay-pocket.log_reference_length', 12)],
      ]);
      
      $reference = config('pay-pocket.reference_string_prefix', '');
      
      $reference .= isset($refGen[0], $refGen[1])
          ? $refGen[0]::{$refGen[1]}(...$refGen[2] ?? [])
          : Str::random(config('pay-pocket.log_reference_length', 12));`
    

@HPWebdeveloper
Copy link
Owner Author

@3m1n3nc3

Thank you for sharing your feedback and concerns and the attention to detail in highlighting the impact of removing optional parameters for the log reference generator. It is truly valuable, as will help make improvements to this open source project.

I will carefully consider your suggestion to ensure that we maintain the flexibility and functionality that the users have come to rely on.

Please rest assured that your feedback is important to me, and I am committed to consider or delivering a solution that addresses these concerns. If you have any further thoughts or ideas, please don't hesitate to share here. I am always eager to hear and work together to enhance the project.

@HPWebdeveloper HPWebdeveloper mentioned this pull request Jun 26, 2024
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.

3 participants