-
Notifications
You must be signed in to change notification settings - Fork 490
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
3950 Dev Guide gives more guidance on type safety, code quality, code style, etc #3971
Conversation
…s for people not using it Updated documentation.rst to move the better way of installing sphinx first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oscardssmith thanks for making this pull request but I think I'm going to write this myself after all. Thanks for opening #3950! Let's talk about some of the stuff in my review. Also, rather than 3950-Dev-Guide-should-give-more-guidance-on-type-safety,-code-quality,-code-style,-etc
I would have gone for a branch name like 3950-dev-guide
. 😄 For branch naming standards, please see http://guides.dataverse.org/en/4.7/developers/version-control.html#create-a-new-branch-off-the-develop-branch .
Like all development teams, the `Dataverse developers at IQSS <https://dataverse.org/about>`_ have their habits and styles when it comes to writing code. Let's attempt to get on the same page. :) | ||
|
||
.. contents:: |toctitle| | ||
:local: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want a table of contents on every non-index page. The rule is at http://guides.dataverse.org/en/4.7/developers/documentation.html#table-of-contents and was added as part of #3796.
NetBeans | ||
^^^^^^^^ | ||
|
||
We highly recommend using Netbeans as a text editor. This makes it significantly easier to commit code that is well formatted and type safe. By default, NetBeans will warn you about some things, but if you want better warnings and code formatting, download this##################TODO ADD FILE######################### zip file, and import it to your nNtBeans settings by going to options, import, and selecting the file. These settings will cause NetBeans to automatically format code that you've worked on whenever you save, point out many common problems in code, and make sure that all indentation uses spaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly want this zip file. I don't trust other developers.
|
||
As you probably gathered from the :doc:`dev-environment` section, IQSS has standardized on Netbeans. It is much appreciated when you format your code (but only the code you touched!) using the out-of-the-box Netbeans configuration. If you have created an entirely new Java class, you can just click Source -> Format. If you are adjusting code in an existing class, highlight the code you changed and then click Source -> Format. Keeping the "diff" in your pull requests small makes them easier to code review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bit about "keeping the 'diff' in your pull requests small makes them easier to code review" is very important to me. I don't want a new developer to join the project and say, "Hey, the coding style pages says we use spaces, so I reformatted the entire code base to remove tabs. Here's a pull request." I want a new developer to focus on fixing an actual bug or improving the user experience for end users with a small "diff", a small pull request.
When working on code, you should only reformat the code you're working on. Likewise, you should only improve the type safety of the code you're working on. Eventually, we'll make the code consistent and of higher quality. It won't happen in one summer.
|
||
As you probably gathered from the :doc:`dev-environment` section, IQSS has standardized on Netbeans. It is much appreciated when you format your code (but only the code you touched!) using the out-of-the-box Netbeans configuration. If you have created an entirely new Java class, you can just click Source -> Format. If you are adjusting code in an existing class, highlight the code you changed and then click Source -> Format. Keeping the "diff" in your pull requests small makes them easier to code review. | ||
For an example code that follows our styleguide, see this | ||
############TODO INSERT IMAGE################################ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of an image, let's inline a file. For an example of three downloadable JSON files, see http://guides.dataverse.org/en/4.7/installation/oauth2.html#dataverse-side
- Error Fixes | ||
- JDK Migration Support | ||
- Probable Bugs | ||
- Standard Javac Warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use all of these. Let's have a conversation about which to turn on and why.
|
||
Alternative option (Mac/Unix/Windows): | ||
|
||
Unless you already have it, install pip (https://pip.pypa.io/en/latest/installing.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mac users need to be told to install pip, right?
I spoke with @oscardssmith about this just now and he agreed that we'll close this pull request. I'll incorporate all the good ideas in here in future branch with #3950 in the name. Thanks, @oscardssmith ! |
Updated coding-style.rst with more info about using netbeans, and type safety tips
Updated documentation.rst to move the better way of installing sphinx first.
This commit improves our developer documentation for writing good code.
Related Issues
Pull Request Checklist