-
Notifications
You must be signed in to change notification settings - Fork 582
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
Document logisim.data package #1863
base: main
Are you sure you want to change the base?
Conversation
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 do not see the point of this PR. While LSe' code style is far from perfect, it is somehow consistent. The type of changes introduced in this PR add no value and can be made easily with formatting tools, not to mention there's no point of changing just single file. I'd vote to reject this one.
Thanks for the feedback. I have implemented these changes mostly to reduce the amount of style warnings coming from the @MarcinOrlowski would such improvements to the PR be welcome? |
Since @MarcinOrlowski introduced PRs with similar scopes (#1853, #1854, #1856, #1857), they should be welcome. Adding documentation to the project is one of the best things you can do. Unfortunately, LSe is lacking severely in this area. |
@cosineblast When we first introduced linters years ago, we opted for CheckStyle with Google's built-in coding style as a base, however, we made several exceptions to the rules. This wasn't due to quality in LSe's code only, but rather because not all rules in Google's style guide were aligned with what we wanted to enforce. It's important to note that there's no one-size-fits-all coding style, and LSe is no exception. It's also worth pointing out that we adapted our code style policy to existing code style in some portions (like brackets) because it was already widely spread across the code base. While I personally also prefer the style with brackets, there's a reason we allow bracket-less branches and I would most likely write new code the same way here for sake of consistency. Our project's code style is evolving as we make incremental improvements through refactoring and any attempts to enhance code quality are welcome, but the reason I am objecting here is that this PR introduces changes without necessarily improving anything. Adding brackets is a trivial operation and hasn't been done for years, because the current code style is simply consistent. Will that be changed in future? Maybe, but I dare to say the "evolution" over "revolution" is better choice, and since we use no preprocessor nor anything that would unfold one-liner into bigger multi-instruction code block ruining the logic, adding brackets here only bloats the code without any tangible benefit: The PRs that @R3dst0ne linked are not directly relevant here. What he failed to notice is that those PRs aim to reduce verbosity and cut down lines of code, not the opposite. If you have some spare cycles to contribute to the LSe project, you're more than welcome. However, more beneficial refactoring tasks would include using |
This makes sense. I'll be updating the PR in the following days to provide more meaningful changes (i.e documentation and final vars) to this and other classes in the |
@cosineblast: Thanks for your willingness to contribute! Every PR is welcome, which helps to improve Logisim-evolution! You don't need to close this PR to rework it, but rather mark it as draft and inform us, when you would appreciate some feedback. Please, keep PRs topical and if possible at a manageable size for review. Avoid forces pushes once people have made review comments to code sections. Otherwise, GitHub looses track, which files didn't change since the last review. |
Alright, I will do so. |
I have updated this pull request to provide documentation for
There are also a few unused methods.
|
This PR (draft) contains style fixes for the class Value.java. It contains/will contain fixes for the incorrectly braced if-else statements, one logic change (to get rid of an empty brace warning) and javadocs for all public methods.
Small question:
While documenting the public functions, I encountered this 'controls(' function. It doesn't seem to be used anywhere in the project, and i'm not particularly sure of what it does. Is it ok to remove it?