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

perlcritic, readability, sprintf instead of concatenation #49

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@bieron
Contributor

bieron commented Oct 24, 2015

Hello!

I received Your module via CPAN Pull Request Challenge '15.
Here are some minor (mostly readability) improvements. I'm sorry I wasn't able to help with any real issues, like Windows portability for example, but I am not experienced with XS.

cpanpr #hacktoberfest

@autarch

This comment has been minimized.

Contributor

autarch commented Oct 24, 2015

The modules in inc are simply copied from other CPAN distros. We don't want to change them.

@2shortplanks

This comment has been minimized.

Member

2shortplanks commented Oct 24, 2015

What @autarch means to say is the modules in inc are automatically copied into here as part of the build process; It's not just we've copied and pasted code, but utility code Dist::Zilla has installed in our distribution. Code in inc isn't installed on the machine when the distribution is installed either - it's code that's actually used during the install / testing process itself.

We typically commit this generated code so we can keep track of what code we're actually releasing to the CPAN in version control, but we're not actually maintaining it ourselves.

@autarch

This comment has been minimized.

Contributor

autarch commented Oct 24, 2015

@2shortplanks, no, that's not right. The modules in inc were literally copied from CPAN distros. I'm not 100% sure that's necessary these days with configure_requires in the meta-info.

@bieron

This comment has been minimized.

Contributor

bieron commented Oct 24, 2015

Wow, thanks for the fast response! I vaguely suspected inc nature. I'd like to know if you'd want to merge my request, provided I removed said existence checks and any changes from inc? And if so, do you want a new request, or should I revert said changes in this one?

};
push @pointer_thresholds,
});
for (19, 27) {

This comment has been minimized.

@autarch

autarch Oct 24, 2015

Contributor

I'm not sure this is any clearer than what it replaces.

@autarch

This comment has been minimized.

Contributor

autarch commented Oct 24, 2015

@bieron - Yes, the other changes look fine (except for the one I just commented on). You can either close this PR and make a new one or rebase your branch with a new set of changes. It's up to you. The first option will be simpler if you're not familiar with git rebasing.

@bieron bieron changed the title from perlcritic, readability, existence checks to perlcritic, readability, sprintf instead of concatenation Oct 25, 2015

trivial changes
- change skip map to a 'ne' test
- building more readable label with sprintf
- no context masking of my $position
@autarch

This comment has been minimized.

Contributor

autarch commented Oct 26, 2015

Thanks, I ended up merging this manually from the CLI with a few extra tweaks.

@autarch autarch closed this Oct 26, 2015

@bieron

This comment has been minimized.

Contributor

bieron commented Oct 27, 2015

You're welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment