[MIG] base_geoengine: Migration to 18.0#1
Open
weinni2000 wants to merge 1 commit into18.0.after_lintersfrom
Open
[MIG] base_geoengine: Migration to 18.0#1weinni2000 wants to merge 1 commit into18.0.after_lintersfrom
weinni2000 wants to merge 1 commit into18.0.after_lintersfrom
Conversation
Reviewer's Guide by SourceryThis pull request migrates the base_geoengine module from Odoo 17.0 to 18.0. The main changes include updating the JavaScript code to be compatible with Odoo 18.0's new JavaScript framework, fixing template issues, and updating database-related code to work with the new version. Class diagram for updated geoengine expressionsclassDiagram
class GeoField {
+column_type()
+convert_to_column(value, record, values, validate)
}
class GeoOperator {
+_get_direct_como_op_sql(table, col, value, params, op)
+_get_postgis_comp_sql(table, col, value, params, op)
+get_geo_greater_sql(table, col, value, params)
+get_geo_equal_sql(table, col, value, params)
}
class Expression {
+parse()
+__leaf_to_sql(leaf, model, alias)
}
GeoField --> GeoOperator : uses
Expression --> GeoOperator : uses
Expression --> GeoField : uses
Class diagram for updated GeoengineRecord componentclassDiagram
class GeoengineRecord {
+setup()
+createRecord(props)
+static template
+static Compiler
+static INFO_BOX_ATTRIBUTE
}
GeoengineRecord --> GeoengineCompiler : uses
GeoengineRecord --> INFO_BOX_ATTRIBUTE : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @weinni2000 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| createRecord(props) { | ||
| const {record} = props; | ||
| this.record = Object.create(null); | ||
| this.record = Object.create(null); // Kills the Chrome debugger |
There was a problem hiding this comment.
issue (bug_risk): Problematic workaround for Chrome debugger issue
Using Object.create(null) as a workaround for Chrome debugger issues is concerning. This could cause maintainability problems and debugging difficulties. Consider finding a proper solution that doesn't break debugging tools.
base_geoengine/README.rst
Outdated
| - A search bar is also available. It allows you to perform a search | ||
| into the RecordsPanel. | ||
| - A button to open/close the panels is also available. | ||
| - The module has been translated in French. |
There was a problem hiding this comment.
issue (documentation): Remove duplicate word 'now'
| expression.SQL_OPERATORS.update(GEO_SQL_OPERATORS) | ||
|
|
||
|
|
||
| def __leaf_to_sql(leaf, model, alias): # noqa: C901 |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] (
remove-redundant-slice-index) - Move assignments closer to their usage (
move-assign) - Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Low code quality found in __leaf_to_sql - 5% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Comment on lines
+992
to
+997
| if operator in (">", "<="): | ||
| right += " 23:59:59" | ||
| else: | ||
| right += " 00:00:00" |
There was a problem hiding this comment.
issue (code-quality): Replace if statement with if expression (assign-if-exp)
Comment on lines
14
to
+17
| else: | ||
| base = self.geo_field.entry_to_shape(value, same_type=False) | ||
| params.append(base.wkt) | ||
| return f" ST_Area({table}.{col}) {op} ST_Area(ST_GeomFromText(%s))" | ||
| return f" ST_Area({table}.{col}) {op} ST_Area(ST_GeomFromText('%s'))" |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary else after guard condition (remove-unnecessary-else)
Suggested change
| else: | |
| base = self.geo_field.entry_to_shape(value, same_type=False) | |
| params.append(base.wkt) | |
| return f" ST_Area({table}.{col}) {op} ST_Area(ST_GeomFromText(%s))" | |
| return f" ST_Area({table}.{col}) {op} ST_Area(ST_GeomFromText('%s'))" | |
| base = self.geo_field.entry_to_shape(value, same_type=False) | |
| params.append(base.wkt) | |
| return f" ST_Area({table}.{col}) {op} ST_Area(ST_GeomFromText('%s'))" |
weinni2000
force-pushed
the
18.0-mig-base_geoengine
branch
3 times, most recently
from
fa710ed to
72b900b
Compare
weinni2000
force-pushed
the
18.0-mig-base_geoengine
branch
from
72b900b to
490d05a
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
see modifications necessary for v18
Summary by Sourcery
Migrate the base_geoengine module to Odoo version 18.0, updating SQL expression handling, refactoring JavaScript modules, and adjusting documentation and CI configurations.
Enhancements:
Build:
CI:
Documentation: