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

Tidy up all files #353

Merged
merged 2 commits into from Jun 30, 2018

Conversation

Projects
None yet
2 participants
@ldidry
Contributor

ldidry commented Jun 23, 2018

The interest in having perltidy tests in Travis is to check for non-tidy code introduction, but it can only be achieved if Sympa itself passes the test. This PR makes Sympa passes the test successfully.

Use git diff --ignore-all-space on the commit to show only non-spaces changes (there is a lot of space changes)

@@ -965,7 +963,9 @@ sub _merge_changes_paragraph {
sub get_id {
my $that = shift->{context};
(ref $that eq 'Sympa::List') ? $that->get_id : (defined $that) ? $that : '';
(ref $that eq 'Sympa::List') ? $that->get_id

This comment has been minimized.

@ikedas

ikedas Jun 25, 2018

Member

Weird indentation (I tried perltidy-ing again, but this was not fixed). Bug (or feature) of Perl::Tidy?

@ikedas

ikedas Jun 25, 2018

Member

Weird indentation (I tried perltidy-ing again, but this was not fixed). Bug (or feature) of Perl::Tidy?

This comment has been minimized.

@ldidry

ldidry Jun 25, 2018

Contributor

Well, IMHO, that's not the weirdest indentation that Perl::Tidy asked for. I'm not a big fan of src/lib/Sympa/Database.pm L.77 for ex.

Also, I can't find it anymore (too much changed files), but I remember there's an if that have its closing ) { not on the same level of indentation and that was weird since Perl::Tidy made me change a lot of closing ) { indentation.

@ldidry

ldidry Jun 25, 2018

Contributor

Well, IMHO, that's not the weirdest indentation that Perl::Tidy asked for. I'm not a big fan of src/lib/Sympa/Database.pm L.77 for ex.

Also, I can't find it anymore (too much changed files), but I remember there's an if that have its closing ) { not on the same level of indentation and that was weird since Perl::Tidy made me change a lot of closing ) { indentation.

This comment has been minimized.

@ikedas

ikedas Jun 28, 2018

Member

I see. How weird perltidy does, it does its best.

@ikedas

ikedas Jun 28, 2018

Member

I see. How weird perltidy does, it does its best.

@ikedas

This comment has been minimized.

Show comment
Hide comment
@ikedas

ikedas Jun 25, 2018

Member

I checked with Perl 5.26.0 + Perl::Tidy-20180220, and all on modules were OK.

  • Executables would also be better to be tidied.

I executed as below:

$ perltidy -pro=doc/dot.perltidyrc -it=2 -b \
cpanfile \
src/*/*.{pl,fcgi}.in \
`find src -name '*.pm'` \
src/lib/Sympa/Constants.pm.in
Member

ikedas commented Jun 25, 2018

I checked with Perl 5.26.0 + Perl::Tidy-20180220, and all on modules were OK.

  • Executables would also be better to be tidied.

I executed as below:

$ perltidy -pro=doc/dot.perltidyrc -it=2 -b \
cpanfile \
src/*/*.{pl,fcgi}.in \
`find src -name '*.pm'` \
src/lib/Sympa/Constants.pm.in
@ldidry

This comment has been minimized.

Show comment
Hide comment
@ldidry

ldidry Jun 25, 2018

Contributor

Just a thought: we could avoid quite a lot of carriage return just by using a max line width of 80 cols. It may improve readability. Should we use that value?

Contributor

ldidry commented Jun 25, 2018

Just a thought: we could avoid quite a lot of carriage return just by using a max line width of 80 cols. It may improve readability. Should we use that value?

@ikedas

This comment has been minimized.

Show comment
Hide comment
@ikedas

ikedas Jun 28, 2018

Member

I forgot exact reason, but there might be a consideration for general terminals. On 80-cols display, newline of a line with 80-cols might run over into the next line.

Member

ikedas commented Jun 28, 2018

I forgot exact reason, but there might be a consideration for general terminals. On 80-cols display, newline of a line with 80-cols might run over into the next line.

@ldidry

This comment has been minimized.

Show comment
Hide comment
@ldidry

ldidry Jun 28, 2018

Contributor

Ok.

Contributor

ldidry commented Jun 28, 2018

Ok.

@ikedas ikedas merged commit 4f949af into sympa-community:sympa-6.2 Jun 30, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment