Dummy PR to check codacy #157
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @damithc, your pull request title is invalid. It should be in the format of Submit only one activity per pull request (unless otherwise stated in instructions) and do remember to create your branches from the commit where the Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at cs2103-pr-bot and add a link to this PR. |
damithc
pushed a commit
that referenced
this pull request
Nov 19, 2016
* Added checkstyle plugin for eclipse and add some documentation on how to use it. * Attempted to fix invalid values in checkstyle configuration. Added more detailed instructions to setup checkstyle after project import. * Added localised checkstyle-related files to gitignore. * Removed eclipse plugin settings files. * Added tip to click on the text again after selection.
* UiPart: move setIcon() into MainWindow UiPart::setIcon() is only used by MainWindow. Furthermore, UiParts should not be allowed to change the primary stage's icon at their whim and fancy, as it may lead to multiple UiParts conflicting with each other because they all want to set the primary stage's icons. So, make it clear that only MainWindow is allowed to do this by moving UiPart::setIcon() into MainWindow, and making it private. * UiPart: move setIcon() into FxViewUtil UiPart::setIcon() does not depend on any internal data of UiPart, and thus is better suited to be in an external utility class as a static method. This is done to make it clear that calling setIcon() will not modify the state of any objects, besides the Stage passed in as an argument. * FxViewUtil: rename setIcon() to setStageIcon() This is done to make it clear that the method operates on a Stage. * UiPart: push down primaryStage Currently, all UiParts store a reference to their primaryStage, which means that they can modify the primaryStage at their own whim and fancy. We want to prevent that -- UiParts should not be able to access the primaryStage as it makes it hard to reason about which UiPart is controlling the primaryStage and which is not. As such, we want to remove the reference to the primaryStage from all UiParts. Of all the UiParts, we note that only MainWindow requires a reference to the primaryStage. All other UiParts do not, and SHOULD NOT, keep a reference to the primaryStage. So, we push down the `primaryStage` field from UiPart to MainWindow only. All other UiParts thus effectively lose access to the primaryStage, which is a good thing for encapsulation. * UiPartLoader: rename loadUiPart() to initUiPart() All other variants of loadUiPart() will reconstruct the UiPart based on the value of "fx:controller" in its FXML file. Only loadUiPart(T seedUiPart) does not -- it simply sets up the FXML scene graph using the existing UiPart object provided. To highlight the difference in behaviour of this method from the others, rename this method to `initUiPart()`. * UiPartLoader: add single argument variant of loadUiPart()
The `run` and `runShadow` tasks provide a convenient way for developers to run the built application from its *.class files or the built fat JAR respectively. This is accomplished using the application plugin[1] which implements the 'run' task. The shadow plugin, which we already use, will automatically detect the `application` plugin and will integrate with it[2], providing the `runShadow` task. It also knows that `mainClassName` should be used as the "Main-Class" of the JAR manifest, so we don't need to specify that anymore. We add the `mainClassName` and `plugin` lines OUTSIDE of the `allprojects` block because the question of "which class should be run" is Gradle project-specific and only applicable to the root project. [1] https://docs.gradle.org/current/userguide/application_plugin.html [2] http://imperceptiblethoughts.com/shadow/#integrating_with_application_plugin
* codestyle: remove trailing whitespace Some lines in the code look like an empty line, but actually contain spaces. To be consistent with the rest of the code, let's make sure all empty lines are actually empty. * checkstyle: check for trailing whitespace To ensure that no more trailing whitespace is introduced into our code base, add a check for it.
StatusBarFooter currently constructs and sets up its StatusBar controls using pure code. The StatusBar control is actually a real control that can be initialized directly in the FXML file. We can avoid placeholder elements and make things simpler by just doing that. Now, one may think: "This means Scene Builder cannot edit this FXML file any more!", since the StatusBar control comes from controlsfx, which is not supported by Scene Builder by default. However, 1. Scene Builder can actually be configured[1] to support the custom controls from controlsfx, allowing it to edit the FXML file. 2. Having too many placeholder nodes, and constructing the actual scene graph in code, kind of defeats the advantage of FXML which is to construct the scene graph declaratively without code. [1] http://stackoverflow.com/a/30078204
Checking out this repo on a Windows machine with a vanilla Git config,
then running `gradlew.bat checkstyleMain` gives the following errors:
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\MainApp.java:0: File does not end with a newline. [NewlineAtEndOfF
ile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\ComponentManager.java:0: File does not end with a new
line. [NewlineAtEndOfFile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\Config.java:0: File does not end with a newline. [New
lineAtEndOfFile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\EventsCenter.java:0: File does not end with a newline
. [NewlineAtEndOfFile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\GuiSettings.java:0: File does not end with a newline.
[NewlineAtEndOfFile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\LogsCenter.java:0: File does not end with a newline.
[NewlineAtEndOfFile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\Messages.java:0: File does not end with a newline. [N
ewlineAtEndOfFile]
[ant:checkstyle] [ERROR] C:\Users\pyokagan\pyk\addressbook-level4\src\main\java\
seedu\address\commons\core\UnmodifiableObservableList.java:0: File does not end
with a newline. [NewlineAtEndOfFile]
...etc
This goes on for every single Java file.
This is because this repo's files uses LF as the line separator.
However, the default "lineSeparator" value of the "NewlineAtEndOfFile"
check is CRLF on Windows[1], hence these errors.
One may argue that we could switch on `core.autocrlf` in Git, which
would tell Git to automatically convert all LF line endings to CRLF,
thus avoiding this problem. However, this not only places an additional
burden on Windows users to configure Git properly, it also does not
handle the use cases where a developer is not using Git. The developer
may, for example, instead get hold of the repo by downloading the ZIP
file using Github's "Download as ZIP"[2] functionality, and will thus
still have this problem as the downloaded ZIP contents will still
contain LF line endings.
Thus, we should either switch the "lineSeparator" property to "lf",
accepting only LF line endings, or "lf_cr_crlf", accepting LF, CR or
CRLF line endings. (Checkstyle does not support just LF and CRLF only)
However, some developers may have `core.autocrlf` accidentally set, or
may even prefer to use CRLF line endings in their working copy. As such,
let's go the route of allowing LF, CR or CRLF as line endings.
[1] http://checkstyle.sourceforge.net/config_misc.html#NewlineAtEndOfFile
[2] https://github.com/se-edu/addressbook-level4/archive/master.zip
) * codestyle: ensure curly brackets are separated by whitespace The Java coding standard[1] requires that curly brackets ({}) be surrounded by whitespace. However, there are several violations of this requirement in the code base. e.g. protected void raise(BaseEvent event){ -------------------------------------^ Space required before '{' Fix all of these code style violations. [1] https://oss-generic.github.io/process/codingstandards/coding-standards-java.html#methods * checkstyle: ensure curly brackets have whitespace around them There used to be methods in the code base where the curly brackets ({}) are not separated by whitespace: protected void raise(BaseEvent event){ -------------------------------------^ Space required before '{' Our checkstyle configuration failed to catch these errors because we did not tell checkstyle to check for the LCURLY, RCURLY and SLIST (statement list) tokens. Fix this in the checkstyle config file so that such code style violations will not happen again.
* Construct BrowserPanel using FXML The DeveloperGuide states that the layout of individual UiParts are specified in matching `.fxml` files. e.g. The layout of `MainWindow.java` is specified in `MainWindow.fxml`. While this is true for almost all UiParts, there is one oddball UiPart that does not have a corresponding `.fxml` file -- BrowserPanel. For consistency with the rest of the UiParts, let's construct the BrowserPanel using FXML rather than using code. * Remove DefaultBrowserPlaceHolderScreen.fxml It is not being used at all.
AppVeyor[1] is a continuous integration service used to build and test projects on a Windows VM. Adding AppVeyor support to this project will allow us to easily ensure that our tests do not break on Windows. Add an appveyor.yml file that will teach AppVeyor how to build this project and run its tests. Update our documentation to explain to developers how to set up AppVeyor on their own repositories. For now, we will run our tests with 64-bit Java only, since the project does not use any native libraries and thus is unlikely to have any architecture-specific bugs. In the future, should we use any native libraries, the appveyor.yml file should be updated to run our tests on both 32-bit and 64-bit Java. [1] https://www.appveyor.com/
* Modify GuiHandle::getNode() return type Also removed type-castings on calls to this function in GUIHandles * Refactor codes in GUIHandler to ultilise the new getNode function * Modify GuiHandle::getNode() to use query instead of its tryQuery counterpart * Remove cast * Remove unused method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
as per title