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

Coding style checker #21

Closed
kenjis opened this issue Mar 15, 2016 · 23 comments
Closed

Coding style checker #21

kenjis opened this issue Mar 15, 2016 · 23 comments
Milestone

Comments

@kenjis
Copy link
Member

kenjis commented Mar 15, 2016

Is there coding style checker (and/or fixer) for CI4 like php-cs-fixer?

@lonnieezell
Copy link
Member

One does not exist, currently. I've got the settings generally in my IDE (PHPStorm) but haven't messed with anything else yet. Honestly, don't have experience with those, either.

@kenjis
Copy link
Member Author

kenjis commented Mar 16, 2016

I want the official coding style checke/fixer for CI4.
If we have it, we can check the style automatically, and we could reject bad style PR automatically. ;-)

@lonnieezell
Copy link
Member

Yes, it would be nice, and we can probably get to that at some point. :)

@lonnieezell lonnieezell modified the milestone: Pre-Alpha 2 May 9, 2016
@jim-parry
Copy link
Contributor

@kenjis Is the current styleguide workable for you?
If so, it would be great, IMHO, to add the style check to the travis-ci build :)

@lonnieezell
Copy link
Member

I found this site this morning that looks like a great way to integrate: https://styleci.readme.io/

I've signed up for the repo but haven't integrated, yet. Will try to do that in the next day or so.

@mwhitneysdsu
Copy link
Contributor

I made some minor changes to the styleguide to correct some errors. Most of the better coding style checkers/fixers allow the user to configure the enforced rules, so we can probably create (or accept contributions of) the necessary files to support the coding style defined in the guide.

@lonnieezell
Copy link
Member

yes, it supports that - and that is what I haven't configured yet. The plan is to do that tonight if I can break free from work.

@mwhitneysdsu
Copy link
Contributor

I was just thinking in general, as you brought up StyleCI, kenjis brought up php-cs-fixer, and I've done a little work on configuring PHP_CodeSniffer (which I use in SublimeText). I'm not really the kind of person that wants to wait until CI checks run on the repo before I find out that the code doesn't match the style guide, because I know quite well that I'm not going to use Allman-style braces without a tool to remind me (or force me) to do so.

@lonnieezell
Copy link
Member

Completely understand. StyleCI seems to run codesniffer (I think, looking at the "fixer" names), but provides tools to generate pull requests to fix common items, etc. Also, it integrates right into GitHub, much like Travis does, so we can see if there are errors before merging. Seems ideal for working with lots of contributors.

I've got PHPStorm setup to handle the styling for me, and had started to setup codesniffer config locally, but never got it finished. That would be nice to have, though.

@lonnieezell
Copy link
Member

I got us setup on StyleCI, but it's throwing an error on our Config/Hooks file. Probably because it's mostly a placeholder file. I've got an email into their support, so we'll where that takes us. It doesn't support all of the different things our styleguide does, but it will help keep many things standardized.

@jim-parry
Copy link
Contributor

Looking forward to where this takes us!

@jim-parry
Copy link
Contributor

The updated styleci config still fails, and now when I click the badge, and then "show details", it shows the sorce code changes without an indication of why something might have failed the style check?

@lonnieezell
Copy link
Member

lonnieezell commented Jun 6, 2016

Correct. I tweaked the styles last night to remove a good portion of the
extra whitespace checks. It will show fail until we resolve the current
errors, which is a matter of confirming that the styles being applied are
what we want, and then having it submit a PR to update them. I only got to
look through about 20% of the file last night, but it was looking pretty
good.

@lonnieezell
Copy link
Member

While StyleCI does some cool things for us, there are a few things it does that don't work and I haven't found the checkers that control that, so I think I'll remove it for now. We can re-evaluate later, or look at different tools.

@jim-parry
Copy link
Contributor

Agreed.

@timw4mail
Copy link

Just a thought: Editorconfig is a popular way to enforce basic coding style at an editor level. It just involves a .editorconfig file in the root, and would act as a passive starting point for code style.

Here's an example of a fairly general .editorconfig file:

# EditorConfig is awesome: http://EditorConfig.org

# top-most EditorConfig file
root = true

# Unix-style newlines with a newline ending every file
[*]
end_of_line = lf
insert_final_newline = false
charset = utf-8
indent_style = tab
trim_trailing_whitespace = true

[*.{cpp,c,h,hpp,cxx}]
insert_final_newline = true

# Yaml files
[*.{yml,yaml}]
indent_style = space
indent_size = 4

@mwhitneysdsu
Copy link
Contributor

While Editorconfig looks promising, most of the settings it supports are included in your example, which isn't going to help with most of the things that come up with coding style issues. One of the glaring issues for this project is that it does not support configuring the use of tabs for indentation while using spaces for alignment.

@timw4mail
Copy link

@mwhitneysdsu I'm aware it isn't very powerful, but it at least has the potential to help get rid of space indentation.

@mwhitneysdsu
Copy link
Contributor

@timw4mail It may get rid of space indentation, but tabs used in alignment are a significant problem as well, and more difficult to correct if you don't have a script that can correct it for you.

Personally, I only use CI4's coding style when working on CI4 itself, so I've spent a lot of time trying to configure my editor and linter to behave properly in each of my projects, so it would be nice if this were as simple as downloading a file with the project. However, I'm still going to need to install/configure a linter and some other plug-ins to really check/enforce the style guide.

I think Editorconfig is a good idea for some users, but there are better ways of dealing with this problem, in my opinion. Ideally, if we could get a good configuration for PHP CS Fixer, it wouldn't matter what the individual users' editor settings were, because PHP CS Fixer would be able to do its job and reformat the file to meet the requirements.

Until then, I just use the following project file in SublimeText (actually, this is simplified a bit), along with some rules-in-progress for PHP CodeSniffer (which is run by SublimeText on save) to tell me when things aren't right.

{
    "folders":
    [
        {
            "path": "/your/path/here"
        }
    ],
    "settings":
    {
        "default_encoding": "UTF-8",
        "default_line_ending": "unix",
        "translate_tabs_to_spaces": false,
        "tab_size": 4,
        "trim_trailing_white_space_on_save": true,
        "ensure_newline_at_eof_on_save": true
    }
}

@groovenectar
Copy link

groovenectar commented Mar 16, 2017

I did notice (in system/Commands) that some files are using 4 indentation characters for a single indent level. That had me concerned until I saw the rest of the project continues to use tabs for tabulation.

Probably sounds weird to say, but it would be heartbreaking if the Codeignitor project in general started using (x number of) space characters to represent a tab character.

PSR. is. wrong. (about that specific issue). And it has influenced many projects in the wrong direction. These are typically web projects, and if we get used to spaces for indentation it could bleed into our HTML/JS and in some cases add +/- 20% to filesize.

Using an .editorconfig file would be a great idea and is very well-supported.

@groovenectar
Copy link

groovenectar commented Mar 16, 2017

Using EditorConfig with PHPStorm, SublimeText and Atom at least, I've not personally run into any issues with spaces for character alignment and tabs for tabulation/indentation. If an editor botches that, then it would have done that anyway, and I feel like that would be really rare. I've not run into that upon opening a file, and EditorConfig isn't designed to change anything upon open/save, just for the editing process. The code style problem is already solved before the file is opened for editing, and before new files are created... I would be curious to know others' findings on that...

@jim-parry
Copy link
Contributor

@lonnieezell Is this superceded by #182 ? It looks like this issue can be closed.

@lonnieezell
Copy link
Member

@jim-parry we should be good with just the other one. Closing.

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

No branches or pull requests

6 participants