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 Plugins for Inernet Port (Well Known Port,Dynamic Port and Regi… #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b03cmans
Copy link

++InternetPort.pm -> return fake random port
++InternetPortDynamic.pm -> return fake Dynamic Port
++InternetPortRegistered.pm -> return fake registered port
++InternetPortWellKnown.pm -> return fake well-known port
// faker.pm changed to support the above

@b03cmans
Copy link
Author

could you help to review and if needed to merge those to support also generation of fake ports

Comment on lines +14 to +22
sub execute {
my ($self, $data) = @_;

return $self->faker->random->bit
? $self->faker->InternetPortDynamic
: ( $self->faker->random->bit
? $self->faker->InternetPortRegistered
: $self->faker->InternetPortWellKnown)
}
Copy link
Owner

Choose a reason for hiding this comment

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

This might be better implemented as:

sub execute {
  my ($self, $data) = @_;

  my $plugin = $self->faker->random->select([
    'internet_port_dynamic',
    'internet_port_registered',
    'internet_port_well_known',
  ]);

  return $self->faker->$plugin;
}

@awncorp
Copy link
Owner

awncorp commented Aug 29, 2022

@b03cmans thanks for the contribution. I agree that this would be a good addition to the core. Whenever you have time, if you could make the following changes it would get this merged!

  • Remove the generated POD from the .pm files
  • Please see the InternetPort suggestion
  • Please move the POD and tests to t/*.t files, e.g. t/Faker_Plugin_InternetPort.t (use this as an example, you can copy/paste most of it)

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.

2 participants