-
Notifications
You must be signed in to change notification settings - Fork 2
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
Initial framework #257
Initial framework #257
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.
LGTM
|
||
class CavesConfig(AppConfig): | ||
default_auto_field = "django.db.models.BigAutoField" | ||
name = "caves" |
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.
LGTM.
), | ||
], | ||
), | ||
] |
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.
LGTM.
|
||
@property | ||
def user(self): | ||
return self.system.user |
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.
LGTM
from django.urls import path # noqa F401 | ||
|
||
app_name = "caves" | ||
urlpatterns = [] |
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.
LGTM.
@@ -0,0 +1 @@ | |||
from django.shortcuts import render # noqa F401 |
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.
LGTM.
to="caves.cavesystem", | ||
), | ||
), | ||
] |
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.
LGTM.
to="caves.caveentrance", | ||
), | ||
), | ||
] |
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.
LGTM.
|
||
operations = [ | ||
migrations.RunPython(create_cave_systems_from_trips), | ||
] |
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.
-
Use of Prints: The code uses
print
statements for progress updates. Consider using Django's logging framework to provide more flexible and configurable log output. -
Inefficient Matching: The
match_trip_to_system
andcreate_new_system
functions can potentially cause a lot of database queries, which may be inefficient for large datasets. Investigate ways to batch queries or optimize lookups. -
No Transaction Management: There is no explicit transaction management. Ensure that all changes are wrapped in database transactions to maintain data integrity.
-
Distance Calculation Assumption: In
match_trip_to_system
, the distance threshold of 10 kilometers (distance_in_km > 10
) might need further validation. Different cave systems can have varying definitions of what constitutes an acceptable distance between entrances. -
Assertions Risks: The use of assertions like
assert all([system, entered_by, exited_by])
can be risky in production because they can be disabled in optimized bytecode. Use appropriate error handling instead. -
Hard-coded Country Mapping: The
match_country
function has hardcoded mappings for "USA" and "united states" only. Consider making this more robust or configurable to handle other common country name variations.
Addressing these points could lead to better performance, enhanced reliability, and more maintainable code.
CaveEntrance, on_delete=models.CASCADE, related_name="exits", null=True | ||
) | ||
|
||
# Old | ||
cave_name = models.CharField( | ||
max_length=100, | ||
help_text="The name of the cave or cave system entered.", |
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.
LGTM.
@@ -96,6 +96,7 @@ def env(name, default=None, force_type: Any = str): | |||
"core.apps.CoreConfig", | |||
"users.apps.UsersConfig", | |||
"logger.apps.LoggerConfig", | |||
"caves.apps.CavesConfig", | |||
"staff.apps.StaffConfig", | |||
"stats.apps.StatsConfig", | |||
"import.apps.ImportConfig", |
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.
LGTM.
@@ -5,6 +5,7 @@ | |||
urlpatterns = [ | |||
path("", include("core.urls")), | |||
path("", include("logger.urls")), | |||
path("caves/", include("caves.urls")), | |||
path("map/", include("maps.urls")), | |||
path("comments/", include("comments.urls")), | |||
path("statistics/", include("stats.urls")), |
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.
There's a potential issue with having multiple routes for path("")
. This could lead to routing conflicts because it's ambiguous which route should handle a root URL ('/'). Consider reviewing the routes to ensure there's no overlap.
Other than that, LGTM.
No description provided.