Inquisitive Investigators #5
Conversation
Fixes Issue #5. Makes paths agnostitic to os.
Most of enigma configs. Not much testing. Any reconfigurations should go towards master branch unless very significant.
Background images and animation
Adds config load + save for lat/lon from geoip, closes #1
|
Lint failing: |
There was a problem hiding this comment.
Summary
A well structured project producing a nice cohesive gui from well structured code that combines a range of different functionalities.
Version control
Commit quality isn't all that great: many commits only have brief, non-descriptive short messages. It looks like most (all?) commits are from two people, both of whom contributed significantly and with a similar style.
General content and code quality
The code is well divided into different modules, both for Python and kv files. Within these, the code quality is generally good, with content divided into appropriate short functions and concerns well separated. The contributions from both primary developers seem well merged, and code style is consistent throughout the project.
The code is in general good, idiomatic Python. Including type hints is a modern touch. There are good attempts to abstract out constants, although there could be more of this, e.g. to pull style information (app colours etc.) from some central source that can be easily updated. There are sporadic docstrings, but usually not very specific.
It's nice to see a range of interactions covered by the code - some sophisticated text and keyboard handling, complex gui control flow managing multiple interactive widgets displayed simultaneously, and various gui mechanisms like popups (with different content) integrated nicely.
Some things worth more attention are:
- There isn't enough documentation. Including some docstrings is good, and in some cases the type hints cover slightly for what would otherwise have to be documented, but there are still plenty of places that would benefit from a clearer description of what's going on. This also applies to the general structure of the app, although it's good that you've split things into multiple submodules you do want to try to be clear about how this has been done, for the benefit of future contributors.
- Similarly with comments, you've generally done a good job keeping methods short and fairly clear, but there are plenty of places where non-obvious things are done that could be clarified.
- There's some questionable control flow, things like returning in the middle of a function that otherwise appears to operate normally. This kind of thing makes things hard to follow. I've commented on a couple of examples.
- It's low-hanging fruit, but I have to call out the
# This file was planned for use, but we ran out of time- you could have deleted it as easily as making that comment :p
Use of Kivy
The code makes very good, varied use of Kivy. Many different widget types (and Behavior mixins) are used to different purposes, and combined well to make a pleasant and well structured user interface that seems to behave well during different types of interactions. The keyboard handling is a nice touch demonstrating a good understanding of how to handle a combination of hotkeys and text input in different places. It's perhaps a small thing, but the consistent loading of small kv files for each python file is a good app structure.
I think some of the splitting of functionality between kv files and Python files merits more explanation - there's nothing enforcing that on a technical level (this is Python after all), but it's a good example of where better documentation could make it clear where concerns are being left to kv and where they're left to Python. A good example would be for widgets where all the functionality is in the kv rule, so the Python side otherwise looks like a trivial class definition. This is a subtler point though that's perhaps partly subjective: it only becomes important in more advanced applications, and isn't something the Kivy documentation emphasises.
README
The README is short, but clear, and includes both installation and usage details. The use of pipenv is a pleasant way to ensure a consistent installation experience. I always like to see a screenshot.
The license of the code is not specified (although it this context it's understood to be inherited from the code jam).
|
|
||
| def on_update( | ||
| self, browser_side: str, | ||
| state: int, files: List[str] |
There was a problem hiding this comment.
Pretty minor, but this would be clearer with 1 argument per line, especially with the type hints adding noise.
| background_color = ColorProperty((0, 0, 0, 1)) | ||
|
|
||
| font_name = StringProperty(FONT) | ||
| font_size = NumericProperty(12.5) |
There was a problem hiding this comment.
Using kivy.metrics.sp(12.5) would be better to guarantee similar results on screens with different dpis.
| so key presses will work again. | ||
| """ | ||
| if self._keyboard is None: | ||
| self.config_keyboard() |
There was a problem hiding this comment.
I'm not sure the keyboard binding was working correctly for me when I tested it, but didn't check in detail, I could be wrong.
| } | ||
|
|
||
| def __getitem__(self, items: Union[str, Tuple]) -> List[int]: | ||
| if isinstance(items, str) or isinstance(items, int): |
There was a problem hiding this comment.
This kind of thing is a bit weird, it might be okay but it's clearly working around some specific details of the way you intend this abstraction to be used. Things like this really benefit from documentation - what exactly is this expected to be indexed with, and why?
There was a problem hiding this comment.
isinstance can take an iterable as a second attribute, so this could be if isinstance(items, (str, int)):
| """ | ||
| if self.prev_selected is not None and self.selected.name == file.name: | ||
|
|
||
| self.selected.alpha = 0 |
There was a problem hiding this comment.
Not to be too picky, but this is a nice example of where the stuff in the if block could be abstracted further to another method. It would be nice to reduce this kind of logic to just if self.prev_selected is not None and self.selected.name == file.name: self._unselect_current_item(). This also makes the control flow clearer, I didn't notice the function could return here until I paid attention to these details of the control structure.
|
|
||
| def __init__(self, *args: Any, **kwargs: Any) -> None: | ||
| super().__init__(*args, **kwargs) | ||
| self.current_dir = str(Path.home()) |
There was a problem hiding this comment.
Using Path.home() exposed some bugs you have: I have a broken symlink in my home dir, which crashes the app since the file parsing isn't robust enough! Something for a test suite to cover, perhaps ;-)
| """ | ||
| Captures specific key presses | ||
| and executes accordingly. | ||
| """ |
There was a problem hiding this comment.
This is a good example of a function that merits some commented explanation - it does quite different things depending on the context, and it's hard to understand this only by looking at the code.
| self.shell.stop() | ||
|
|
||
| if self.cursor_index() < self._cursor_pos: | ||
| return False |
There was a problem hiding this comment.
Quietly returning in the middle of a function is often an antipattern, I think that's the case here. It's easy to miss this if just looking to the bottom of the function to check what it does, and of course it isn't commented or documented elsewhere.
There was a problem hiding this comment.
Quietly returning in the middle of a function is often an antipattern
Sorry, but I'd have to disagree with the above statement; I think that's an over-generalization. It's very often necessary or more efficient to have a return that's in the middle rather than the end, when some of the actions (but not all) need to be completed regardless of the return value.
It can sometimes be indicative of an anti-pattern or poor logical structure if it's not at all necessary or better to return in the middle of the function, but I don't think the early return itself is often an anti-pattern.
That being said, I think the docstring should definitely explain what situation causes the function to return false. But it doesn't have to explain every single return in the middle of the function. The reader can extract that information simply by reading over the code, and seeing where the returns are located.
This may not be the best example of average real-world code that requires early returns (due to complexity), but just for demonstration I was recently working on implementing a new feature in Python 3.9 for a new feature in the concurrent.futures module, specifically adding a new argument for the Executor.shutdown() method.
While implementing the feature for ProcessPoolExecutor, I had to familiarize myself with the inner workings of the function _queue_management_worker(), which has a total of 3 returns in the middle. There's also process_worker(), which has two returns in the middle.
If anyone is curious about the details, see https://github.com/python/cpython/blob/78c7183f470b60a39ac2dd0ad1a94d49d1e0b062/Lib/concurrent/futures/process.py
Note: The feature is currently a PR that's in it's final stages, so it's not yet official. Here's the the link if anyone is curious: python/cpython#18057.
There was a problem hiding this comment.
Fair, maybe I overstated it here. I think it's perfectly reasonable to return in the middle of functions in many circumstances, but I also think it's overused and especially creeps in as a bad practice when making things in a rush - slipping in a return is always short-term-easier than restructuring to make the logic clearer :p.
There was a problem hiding this comment.
Code is Pythonic in places of the project. There is a few issues with naming in places, such as a class called just F which I believe there has to be a better name for. There is also some useless code like saving data to a file and then opening the file again to get that data which while it is not a stylistic Python comment it is not a very Pythonic way of doing things. There are some odd class names such as DIRChoice over DirectoryChoice and Mkdir instead of MakeDirectory or similar. Uppercase variable names are use don occasion which should not be. Legacy syntax is used interchangeably (as mentioned below). There were a few areas where a newline was left after if statements which is a pet peeve of mine.
There are a couple of custom exceptions that have been implemented such as a InvalidBrowser error which is not needed because not only does it trigger regularly but it is almost guaranteed to never trigger as far as I can see. This error and the InvalidSelection error could most likely be replaced with ValueError or if that does not fit then RuntimeError.
In the rex_explorer/core/main.py file there were a lot of config options which could have been converted into a local config file to prevent messy code, as well as this the config options came before the rest of the imports which broke convention.
There was a large usage of legacy syntax for the super function which on it’s own would be okay for back compatibility but there was no need for that here and plus the modern syntax was also used interchangeably with it, an attempt should have been made to unify different syntactical changes to the best of your abilities.
There was a severe lack of documentation among most of the project and while some areas were documented pretty well, for example the utils file, the majority of the code was either documented insufficiently or not at all. There were very few comments as well apart from some commented out code which should not be there.
Usage of modules such as shlex was vey good, though usage could have done with some more comments and documentation explaining what was going on. The usage of a constants file was good, as was the usage of a paths file, but it should probably have been merged with the constants file. Documentation in the utility file was good, but some unused code was commented out and comments were missing.
Commit quality was good overall but certain commits could have done with some better descriptions, commits such as test, refactoring and update README.md & about.txt
README is excellent, contains a screenshot, installation steps for users and developers, licensing information, a summary of the project and overall gave a good introduction to the project.
Pros
- Good use of shlex
- Good use of Kivy language
- Good use of a constants file
- Good use of pathlib
- Code was Pythonic and easy to follow in some files
- Great README
Cons
- Documentation missing on most functions and classes
- Commented out code left in, comments with meaningful text left out
- Some odd class naming
- Legacy syntax and modern syntax used interchangably
| pylint = "*" | ||
|
|
||
| [packages] | ||
| kivy = "*" |
There was a problem hiding this comment.
You should pin your dependencies to a specified version or at least to the next major update. This will prevent undefined behaviour for anyone using your project in future should breaking changes to dependencies be made.
| ## License | ||
|
|
||
| All projects will merged into our Code Jam repository, which uses the [MIT license](../LICENSE). Please make sure that if you add assets, the licenses of those assets are compatible with the MIT license. | ||
| # Rex Explorer - Inquisitive Investigators |
| @@ -0,0 +1,10 @@ | |||
| # This file was planned for use, but we ran out of time | |||
| @@ -0,0 +1,67 @@ | |||
| #:kivy 1.11.1 | |||
| @@ -0,0 +1,56 @@ | |||
| # This file was planned for use, but we ran out of time | |||
|
|
|||
| from typing import Any, Dict, List, Union | |||
| from typing import Any, Tuple, Union | ||
| import os | ||
| import sys | ||
| import shlex |
| @@ -0,0 +1,44 @@ | |||
| from typing import List, Tuple, Union | |||
| @@ -0,0 +1,27 @@ | |||
| from pathlib import Path | |||
|
|
|||
There was a problem hiding this comment.
This could probably have been merged with the constants file, though the use of pathlib here is excellent.
| ) | ||
|
|
||
|
|
||
| def bytes_conversion( |
| t = 'DIR' | ||
|
|
||
| size = '-' | ||
| # size = ' '.join( |
There was a problem hiding this comment.
Unused code should not be commented out, it should be removed.
No description provided.