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

bears/c_languages: Add ArtisticStyleBear #1882

Merged
merged 1 commit into from Jul 21, 2017
Merged

Conversation

yash-nisar
Copy link
Member

@yash-nisar yash-nisar commented Jul 3, 2017

Closes #388

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

.ci/deps.sh Outdated
tar xf ~/astyle.tar.gz -C ~/
cd ~/astyle/build/gcc
make
fi
Copy link
Collaborator

@gitmate-bot gitmate-bot Jul 3, 2017

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • No newline at EOF.

SpaceConsistencyBear, severity NORMAL, section ci.

The issue can be fixed by applying the following patch:

--- a/.ci/deps.sh
+++ b/.ci/deps.sh
@@ -62,4 +62,4 @@
   tar xf ~/astyle.tar.gz -C ~/
   cd ~/astyle/build/gcc
   make
-fi+fi

break_one_line_headers: bool=False,
add_braces_to_one_line_conditionals: bool=False,
remove_braces_from_one_line_conditionals: bool=False,
remove_comment_prefix: bool=True):
Copy link
Member Author

@yash-nisar yash-nisar Jul 4, 2017

Choose a reason for hiding this comment

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

Custom config file argument missing, see http://astyle.sourceforge.net/astyle.html#_Options_File

Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

hm there's --options, or what do you mean?

Copy link
Member Author

@yash-nisar yash-nisar Jul 10, 2017

Choose a reason for hiding this comment

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

I mean I have to provide an argument called as astyle_config_file, will do it in the review iteration. 😉

Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

another PR :)

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

Done 👍

attach_braces_to_namespace: bool=True,
attach_braces_to_class: bool=True,
attach_braces_to_inline: bool=True,
attach_braces_to_extern: bool=False,
Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

settings' names usually start with either allow, prohibit or use would you follow that convention ? this is super important as it is part of settings unification

Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

bump

Copy link
Member

@Makman2 Makman2 Jul 20, 2017

Choose a reason for hiding this comment

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

@yash-nisar bump again :)

Copy link
Member Author

@yash-nisar yash-nisar Jul 21, 2017

Choose a reason for hiding this comment

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

Fixed. 👍

bar()
return 1; }
else
return 0; }
Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

can you shorten this... maybe providing us the list of all ppossible values for this settting along with fewer examples and more tests would do the trick.

Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

I actually don't have something against this longer documentation, but we definitely need some header line that summarizes the possible values and their meanings in a few sentences 👍

return 0; }

:param use_spaces:
In the following examples, q space is indicated with a . (dot), a
Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

q -> q; . -> .

Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

missing documentation ion this parameter, (and this shoulb be said after the doc)

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

Addressed 👍

........bar();
}

For example: If set to ``True``, spaces will be used for
Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

If True spaces will be used.
Example:: (should be a code if not example is not needed

For example: If set to ``True``, spaces will be used for
indentation.
For example: If set to ``False``, tabs will be used for
indentation, and spaces for continuation line alignment as below::
Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

same here

}

:param indent_size:
Specifies the number of spaces per indent.
Copy link
Member

@AsnelChristian AsnelChristian Jul 4, 2017

Choose a reason for hiding this comment

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

Number of spaces per indentation level use the same docs as in the other bears

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

Addressed 👍

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 6, 2017

@Makman2 Please have a look when you're free 😉

@yash-nisar yash-nisar force-pushed the astyle-bear branch 5 times, most recently from dd81188 to 0678801 Compare Jul 6, 2017
@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 6, 2017

Can you help me out with the appveyor build failure ? @Makman2

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 6, 2017

An uncached CircleCI build passes whereas a cached build fails. Note that TravisCI builds pass.

args.append(indent + '=' + str(indent_size))
for k, v in rules_map.items():
if v:
args.append(k)
Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

can be shortened:

args += (k for k, v in rules_map.items() if v)  # don't use [] to prevent creating a intermediate list.

else:
indent = '--indent=tab'
if indent is not None:
args.append(indent + '=' + str(indent_size))
Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

maybe create another dict for these value-parameters?

settings_map = {'--indent': None if use_spaces is None else 'spaces' is use_spaces else 'tab'} # <-- maybe you can beautify the if else expression a bit ;D
# You could also add `--style`

args += (k for k, v in settings_map.items() if v) # or: if v is not None, not sure what's better now

.ci/deps.sh Outdated
wget "https://downloads.sourceforge.net/project/astyle/astyle/astyle%203.0.1/astyle_3.0.1_linux.tar.gz?r=&ts=1499017588&use_mirror=excellmedia" -O ~/astyle.tar.gz
tar -xvzf ~/astyle.tar.gz -C ~/
cd ~/astyle/build/gcc
make
Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

it's not shipped prebuilt? :O

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

It is shipped prebuilt but the version spec is low i.e. 2.03-1, see https://packages.ubuntu.com/trusty/astyle.

Our CI also uses trusty for which we would have to install the version 2.03-1, which is pretty old IMO. For installing the latest version, building it is the only method.

Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

ah kk, but I meant when you download the tar.gz file, aren't there some precompiled binaries inside?

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

Nope, the ...bin folder is generated once we build it.

.ci/deps.sh Outdated
if [ ! -e ~/astyle/build/gcc/bin/astyle ]; then
wget "https://downloads.sourceforge.net/project/astyle/astyle/astyle%203.0.1/astyle_3.0.1_linux.tar.gz?r=&ts=1499017588&use_mirror=excellmedia" -O ~/astyle.tar.gz
tar -xvzf ~/astyle.tar.gz -C ~/
cd ~/astyle/build/gcc
Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

after making astyle you should step out again into the previous working directory to prevent weird errors for coming installations. Or use the -C/--directory flag of make.

"""

LANGUAGES = {'C', 'C++', 'Objective-C', 'C#', 'Java'}
REQUIREMENTS = {DistributionRequirement(apt_get='astyle')}
Copy link
Member

@Makman2 Makman2 Jul 10, 2017

Choose a reason for hiding this comment

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

astyle is also available for dnf in Fedora 26 :)

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 11, 2017

@Makman2 How do the changes look now ?

@Makman2
Copy link
Member

Makman2 commented Jul 11, 2017

@yash-nisar your tests fail :O

'--remove-brackets': remove_braces_from_one_line_conditionals,
'--remove-comment-prefix': remove_comment_prefix
}
args = ('--suffix=none', '--dry-run')
Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

use a list, that's faster to append to

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

Done 👍

if use_spaces is True:
args += (''.join(('-s', str(indent_size))),)
elif use_spaces is False:
args += (''.join(('-t', str(indent_size))),)
Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

why not using another dict for this kind of parameters like I suggested before?

args += (''.join(('-t', str(indent_size))),)
for k, v in rules_map.items():
if v:
args += (k,)
Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

like said before, can be shortened:

args += (k for k, v in rules_map.items() if v)

Copy link
Member Author

@yash-nisar yash-nisar Jul 11, 2017

Choose a reason for hiding this comment

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

Awesome 👍

}
args = ['--suffix=none', '--dry-run']
if bracket_style:
args += (''.join(('--style=', bracket_style)),)
Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

use args.append, makes the command shorter

Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

and I don't think you need to use join for just two values, I think this is easier and better to read ;)

args.append('--style=' + bracket_style)

Copy link
Member

@Makman2 Makman2 Jul 11, 2017

Choose a reason for hiding this comment

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

same below

return 0;
}

For example: ``Banner`` style uses attached, indented braces. Switch
Copy link
Collaborator

@gitmate-bot gitmate-bot Jul 19, 2017

Choose a reason for hiding this comment

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

Line is longer than allowed. (80 > 79)

LineLengthBear, severity NORMAL, section linelength.

return 0;
}

For example: ``Whitesmith`` style uses broken, indented braces. Switch
Copy link
Collaborator

@gitmate-bot gitmate-bot Jul 19, 2017

Choose a reason for hiding this comment

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

Line is longer than allowed. (82 > 79)

LineLengthBear, severity NORMAL, section linelength.

return 0;
}

For example: ``Banner`` style uses attached, indented braces. Switch
Copy link
Collaborator

@gitmate-bot gitmate-bot Jul 19, 2017

Choose a reason for hiding this comment

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

E501 line too long (80 > 79 characters)'

PycodestyleBear (E501), severity NORMAL, section autopep8.

return 0;
}

For example: ``Whitesmith`` style uses broken, indented braces. Switch
Copy link
Collaborator

@gitmate-bot gitmate-bot Jul 19, 2017

Choose a reason for hiding this comment

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

E501 line too long (82 > 79 characters)'

PycodestyleBear (E501), severity NORMAL, section autopep8.

.ci/deps.sh Outdated
@@ -55,3 +55,11 @@ if [ ! -e ~/phpmd/phpmd ]; then
sudo chmod +x phpmd.phar
sudo mv phpmd.phar ~/phpmd/phpmd
fi

# astyle installation
if [ ! -e ~/astyle/build/gcc/bin/astyle ]; then
Copy link
Member

@Makman2 Makman2 Jul 20, 2017

Choose a reason for hiding this comment

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

as you don't seem to cache at all currently, you don't need the if ;)

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 20, 2017

@Makman2 Did you notice that the tests have passed and we have no idea why ? 😛

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 20, 2017

They pass whenever they want to. 😛

Possible values are ``allman, java, kr, stroustrup, whitesmith,
vtk, banner, gnu, linux, horstmann, google, mozilla, pico and
lisp.``
For example: Allman style uses braces that are broken from the
Copy link
Member

@AsnelChristian AsnelChristian Jul 20, 2017

Choose a reason for hiding this comment

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

``Allman style``

Copy link
Member

@Makman2 Makman2 Jul 20, 2017

Choose a reason for hiding this comment

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

Hm not sure, then rather

``allman`` style

to refer to the actual keyword/parameter

Copy link
Member Author

@yash-nisar yash-nisar Jul 21, 2017

Choose a reason for hiding this comment

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

See #1882 (comment). Referring to the actual keyword/parameter may cause problems in some cases like Kernighan & Ritchie. The actual keyword is referred to as kr so replacing Kernighan & Ritchie with kr may make it difficult for users to understand the style name.

@AsnelChristian
Copy link
Member

AsnelChristian commented Jul 20, 2017

aren't they passing locally ?

@Nosferatul
Copy link
Member

Nosferatul commented Jul 20, 2017

@yash-nisar why the tests have passed because of your tests, right?

@Makman2
Copy link
Member

Makman2 commented Jul 20, 2017

@AsnelChristian we had strange caching problems. Cached runs just didn't pass, although everything seemed to be set up correctly (we did the same like for other applications). However we will file an issue to implement caching again, so we can at least get something merged.

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 21, 2017

Updated with the required changes @Makman2 @AsnelChristian

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 21, 2017

Lets see if the tests pass on CI, if they don't I'll remove the if condition in deps.sh.

@yash-nisar
Copy link
Member Author

yash-nisar commented Jul 21, 2017

Bam The problem is now gone 👍 @Makman2

break_one_line_headers: bool=False,
require_braces_at_one_line_conditionals: bool=False,
prohibit_braces_from_one_line_conditionals:
bool=False,
Copy link
Member

@Makman2 Makman2 Jul 21, 2017

Choose a reason for hiding this comment

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

indent by 4 spaces

this is invalid PEP8 it seems

@Makman2
Copy link
Member

Makman2 commented Jul 21, 2017

ack 65d6304

@Makman2
Copy link
Member

Makman2 commented Jul 21, 2017

ack 65d6304

@Makman2
Copy link
Member

Makman2 commented Jul 21, 2017

@rultor merge

@rultor
Copy link

rultor commented Jul 21, 2017

@rultor merge

@Makman2 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 65d6304 into coala:master Jul 21, 2017
@yash-nisar yash-nisar deleted the astyle-bear branch Jul 21, 2017
@rultor
Copy link

rultor commented Jul 21, 2017

@rultor merge

@Makman2 Done! FYI, the full log is here (took me 2min)

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

Successfully merging this pull request may close these issues.

Add bear for astyle
7 participants