Skip to content

Styleguide#5

Merged
10 commits merged intomainfrom
Styleguide
Jun 20, 2022
Merged

Styleguide#5
10 commits merged intomainfrom
Styleguide

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jun 18, 2022

After some research and very limited testing, this is the solution I've come up with for implementing an automated formatter which adheres to a style-guide....specifically, Google's. This automated process only applies to "section 4 Formatting" of the total style guide https://google.github.io/styleguide/javaguide.html. Following the balance of the style guide will need to be manually controlled and will really be up to all of us. Since most of the style guide is debatable, without a clear choice for what is best, I'm ok with whatever we do. My only thing is we all need to do it that same when it comes to naming conventions and design patterns.

@cpapplefamily
Copy link
Copy Markdown
Contributor

I will attempt to follow the rules /Instructions to install manually per the ReadMe. My first question is this the extension pack? https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-java-pack

@cpapplefamily
Copy link
Copy Markdown
Contributor

I'm having a few issues on my side getting the "The active formatter profile does not exist, please check it in the Settings and try again." when trying the Java: Open Java Formatter Settings with Preview

I downloaded your branch and have no issues so its likely something I did when I was poking around.

Also I see you have a few more additions to the settings.json

"[java]": {
    "editor.tabSize": 2,
    "editor.detectIndentation": false,
    "editor.insertSpaces": false
  },
  "editor.formatOnSave": true

cpapplefamily
cpapplefamily previously approved these changes Jun 19, 2022
Copy link
Copy Markdown
Contributor

@cpapplefamily cpapplefamily left a comment

Choose a reason for hiding this comment

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

I have reviewed the procedure and found it working as expected.

@cpapplefamily
Copy link
Copy Markdown
Contributor

cpapplefamily commented Jun 19, 2022

One thing I did find is this dose not work with auto save turned on. That seems to be a know function.

One Suggestion I may add is to merge step 2 and 6 into one step. I can respect the information on what each line is doing but a add these lines to the .vscode/settings.jason

"java.format.settings.url": "eclipse-java-google-style.xml",
"editor.formatOnSave": true

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 19, 2022

One thing I did find is this dose not work with auto save turned on. That seems to be a know function.

One Suggestion I may add is to merge step 2 and 6 into one step. I can respect the information on what each line is doing but a add these lines to the .vscode/settings.jason

"java.format.settings.url": "eclipse-java-google-style.xml",
"editor.formatOnSave": true

Updated the README per the suggestion.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 19, 2022

I started looking at Checkstyle as a means to further help with enforcing the Google style guide. My initial thoughts are we should go with it. I pasted a couple of examples of what it does below. The first is complaining about the names of the member variables. Per the style guide (https://google.github.io/styleguide/javaguide.html#s5-naming) this is proper enforcement. The second example shows that Checkstyle can also properly enforce single line Javadoc (https://google.github.io/styleguide/javaguide.html#s7-javadoc) where there's a block tag and no block tag.

image

image

@cpapplefamily
Copy link
Copy Markdown
Contributor

@ejmccalla can you try something on your machine?

  1. Delete the eclipse-java-google-style.xml file from the root
  2. remove your entries from the .vscode/settings.json
  3. CTRL+SHFT+P and open the USER Settings >Preferences: Open Setting (JSON). Then add this
  "java.format.settings.url": "https://raw.githubusercontent.com/google/styleguide/gh-pages/eclipse-java-google-style.xml",
  "[java]": {
    "editor.tabSize": 2,
    "editor.detectIndentation": false,
    "editor.insertSpaces": false
  },
  "editor.formatOnSave": true,

I wonder if this would then set up the machine to always use the style Guide and would not be project specific. What problems would this create? We could option this in the ReadMe Option 1 Set up project only Option 2 Set up Machine

From my quick investigation I have an observations

Java: Open Java Formatter Settings with Preview Fails You are prompted to either open in read only or download a local copy and edit.
The First [Read Only] works and the "[java]":{...............} entries do override the read-only settings so we can modify if we wish.
The Second Download does just that. It downloads to the folder ".vscode/eclipse-java-google-style.xml", It also adds the line to the .vscode/settings.json " "java.format.settings.url": ".vscode/eclipse-java-google-style.xml", But the editor cant open it.

What issue might this create?

@cpapplefamily
Copy link
Copy Markdown
Contributor

cpapplefamily commented Jun 19, 2022

@ejmccalla > Checkstyle

Is this another module or part of the eclipse-java-google-style.xml

I like Member Variables marked with m_lowerCamelCase
We as a group could discuss this more. I'm sure there is an over-ride

@cpapplefamily
Copy link
Copy Markdown
Contributor

@Nadezaxer @ItsMajestiX @YoloSwagDogDiggity

Any input?

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 19, 2022

Is this another module or part of the eclipse-java-google-style.xml

This is another module...and further experimenting shows that Checkstyle and the Formatter aren't compatible. I found a case where the fight one-another and I'm not interested in resolving this and all of the other discrepancies which will arise.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 19, 2022

I think I've landed on using only Checkstyle as our solution. I like that it is a live linter (users know exactly what rules are being broken and sometimes we won't want an automated tool to change the source) and it's also very customizable (I changed the member name rule for Corey's member variables m_[lowerCamelCase style).

We should move the google_checks.xml configuration file to a library that gets imported into our projects so we don't wind up with lots of different configurations files.

@Nadezaxer
Copy link
Copy Markdown

I would like for the indentation to be 4 spaces instead of two.
Also, in setting.json Linux does not recognizes "java.checkstyle.configuration": "${workspaceFolder}\\google_checks.xml" but it will accept "java.checkstyle.configuration": "${workspaceFolder}/google_checks.xml"

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 19, 2022

I also vote for 4 spaces on indentation. What do other want? @cpapplefamily - I was the only person who didn't follow the member variable convention, so I was outlier :)

Copy link
Copy Markdown
Member

@ItsMajestiX ItsMajestiX left a comment

Choose a reason for hiding this comment

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

I like the use of checkstyle as I was working on integrating it into the build process (#7). The documentation you wrote also looks good so I think this can be merged.

@cpapplefamily
Copy link
Copy Markdown
Contributor

Love the open dialog. Seems like we are really close to approving or first PR as a group.

@cpapplefamily cpapplefamily self-requested a review June 19, 2022 21:44
@cpapplefamily cpapplefamily dismissed their stale review June 19, 2022 21:45

We have found that this is not the system we will be using

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 19, 2022

Corey/Adam - before I merge this PR, are you both ok with doing indentations of 4 spaces?

@cpapplefamily
Copy link
Copy Markdown
Contributor

cpapplefamily commented Jun 20, 2022

Yes I'm good with 4. That is not a hill I would die on. I didn't like 2. Plus I got my m_lowerCammelCase

@ItsMajestiX
Copy link
Copy Markdown
Member

Each time a new block or block-like construct is opened, the indent increases by two spaces. When the block ends, the indent returns to the previous indent level. The indent level applies to both code and comments throughout the block. (See the example in Section 4.1.2, Nonempty blocks: K & R Style.)

https://google.github.io/styleguide/javaguide.html#s4.2-block-indentation

I don't think that would be following google style.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 20, 2022

Google doesn't run the whole world just yet. Until they do, we will exercise our liberty and make changes as we see fit. That means 4 spaces during indents and m_lowerCamelCase for class members. Remember the big picture, we are doing this for the sake of commonality, whether that is Google style to exactly or a slight modification of it will still satisfy our goals. As we progress, I'm sure we will run into other style decisions the team will need to come to an agreement on.

@ItsMajestiX
Copy link
Copy Markdown
Member

That is a good point. The m_ isn't to google style, so 4 spaces is fine.

README.md Outdated

```
"[java]": {
"editor.tabSize": 2,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TabSize 4?

}
"java.test.defaultConfig": "WPIlibUnitTests",
"[java]": {
"editor.tabSize": 2,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TabSize 4?

Copy link
Copy Markdown
Contributor

@cpapplefamily cpapplefamily left a comment

Choose a reason for hiding this comment

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

I believe you just need to update the Tab Size references in the settings.json and ReadMe

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 20, 2022

I believe you just need to update the Tab Size references in the settings.json and ReadMe

It was a bit more than that...needed to make updates to the indentation section of the Checkstyle configuration file as well.

@ghost ghost merged commit 6b663d6 into main Jun 20, 2022
This pull request was closed.
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.

4 participants