List namespace paths on hover + path-related refactors #484

cammarosano · 2025-11-21T17:12:17Z

No description provided.

cammarosano · 2025-11-21T17:16:58Z

server/tests/test_get_symbol.rs

 }

+#[test]
+fn test_hover_on_namespace_and_module() {


The reason I placed this test here is the similarities with the test above (test_hover_on_model_field_and_method).
But should we have a test file per feature, both tests would end up in a test_hover file.

cammarosano · 2025-11-21T17:31:21Z

server/src/core/import_resolver.rs


 pub fn find_module(session: &mut SessionInfo, odoo_addons: Rc<RefCell<Symbol>>, name: &OYarn) -> Option<Rc<RefCell<Symbol>>> {
-    let paths = (*odoo_addons).borrow().paths().clone();
+    let paths = (*odoo_addons).borrow().path().as_vec();


or .as_multiple, if we're sure that odoo_addons is a namespace?

cammarosano · 2025-11-21T17:33:40Z

server/src/core/odoo.rs

+            let SymbolPath::Multiple(addons_paths) = addons_symbol.borrow().path() else {
+                panic!("Odoo addons symbol paths should be a namespace");
+            };


@fda-odoo we wrote this together.
But since then SymbolPath::as_multiple has been introduced.
Shall I replace this by simply let addon_paths = addons_symbol.borrow().path().as_multiple()?

cammarosano · 2025-11-21T17:36:39Z

server/src/core/odoo.rs

    pub fn add_to_rebuild_arch(&mut self, symbol: Rc<RefCell<Symbol>>) {
        if DEBUG_THREADS {
-            trace!("ADDED TO ARCH - {}", symbol.borrow().paths().first().unwrap_or(&symbol.borrow().name().to_string()));
+            trace!("ADDED TO ARCH - {} - {:?}", symbol.borrow().name(), symbol.borrow().path());


@fda-odoo I had shown you a Symbol::debug_paths() just for this cases.
But I decided to drop it , and use this instead (leveraging the derived debug trait on SymbolPath and always displaying the symbol's name anyway). What do you think?

cammarosano · 2025-11-21T17:40:21Z

server/src/core/import_resolver.rs

+    // source_file_symbol is either a file or a namespace with a single path
+    let source_path = source_file_symbol.borrow().path().as_vec()[0].clone();


This simply keeps the old behavior. But it feels dodgy. What do think?
See also: the previous comment

cammarosano · 2025-11-21T17:41:15Z

server/src/core/import_resolver.rs

        };
-        let source_path = source_file_symbol.borrow().paths()[0].clone();
+        // source_file_symbol is either a file or a namespace with a single path
+        let source_path = source_file_symbol.borrow().path().as_vec()[0].clone();


Same as above (preserves the original behavior). Dodgy?
See also: manual_import

cammarosano · 2025-11-21T17:44:20Z

server/src/core/import_resolver.rs

+    if source_file_symbol.borrow().path().as_vec().len() > 1 {
+        panic!("Importing from a multi-path namespace symbol is not supported in manual_import");
+    }


As a reminder: this makes it possible that source_file_symbol is a namespace symbol (thus, with Multiple paths) in resolve_from_stmt and resolve_import_stmt

fda-odoo

First part review, I didn't review the usages already, only the first 4 commits and the struct SymbolPath.
I'll continue newt week

fda-odoo · 2025-11-21T17:28:58Z

server/src/features/features_utils.rs

        // Possibly add more links in the future
        let Some(typ) = typ.upgrade_weak() else {
-            return S!("")
+            return S!("");


That's not useful :)

oops, sorry, automatic formatting (rust-fmt) 😬

fda-odoo · 2025-11-21T17:43:54Z

server/src/core/symbols/symbol.rs

 }
+
+impl SymbolPath {
+    pub fn as_single(self) -> String {


It is not really intuitive that "as_single" returns a String, and not a Single struct.
If we want a String, maybe

pub fn as_string(self) { self.as_single().map(|s| s.to_owned()) }

would be nice to add? or a match in as_string is ok too.

Or we could rename as_single to get_single_string

According to what we discussed live, iirc you suggested to rename it to "as_single_string", and @mmahrouss suggested "to_single_string", because "as_" suggests returning a reference.
So the last push has "to_single_string" the as name for this method.
I'm fine with both, but I'd probably name it unwrap_single, as it closely relates to Options and Results's unwrap, in which you unwrap the Some/Ok value or panic. In our case, because we have more than one alternative to None (Single and Multiple), we'd have to be specific about what we're unwrapping (unwrap_single or unwrap_multiple).

fda-odoo · 2025-11-21T17:44:53Z

server/src/core/symbols/symbol.rs

+        matches!(self, Self::Single(_))
+    }
+
+    pub fn as_multiple(self) -> Vec<String> {


Same, maybe get_multiple_vector?

cammarosano · 2025-11-21T17:51:14Z

server/src/core/symbols/symbol.rs

+        matches!(self, Self::Multiple(_))
+    }
+
+    pub fn as_vec(self) -> Vec<String> {


This enables us to use the result of .path() just like the old paths() - which is probably a bad thing. 😬

cammarosano · 2025-11-21T18:07:28Z

server/src/core/symbols/symbol.rs

+    pub fn as_single(self) -> String {
+        match self {
+            Self::Single(s) => s,
+            _ => panic!("Not a Single"),
+        }
+    }


This consumes self and returns the String. Which is usually what we want when chaining with .path() (right?).
But then this clashes with Rust's convention of returning references for methods called as_*.
Maybe this should be called to_single or unwrap_single or something like this?
I don't particularly like those names, but it feels closely related to what Option::unwrap does.
Alternatively, should this be returning a &String (in which case Symbol::paths should probably also return &String)?

Instead of displaying the "See also" link, we now list the namespace directories when hovering over a namespace symbol. We also make sure that the documentation section appears before the useful links/directories list section.

The `paths` field in `RootSymbol` has no purpose and is always an empty vector.

Among all symbol variants, adding a path is only relevant for `NamespaceSymbol`. Therefore this commits moved the `add_path` method from the generic `Symbol` enum to the specific `NamespaceSymbol` struct.

Package symbols only have a single path. Before this commit, the `PackageSymbol::paths` method always returned a single-element Vec<String>. This commit renames it to `path` (singular) and makes it return the single path instead.

This commit replaces the Vec<String> return type in Symbol::paths() (now renamed to path) with a new SymbolPath enum that explicitly represents three cases: Single path, Multiple paths, or None. The purpose of this change is to force the caller handle the different cases explicitly, rather than doing [0] on the vector previously returned by paths(). This commit also renames get_symbol_first_path() to get_file_path() for clarity, as it only applies to file types (File, Package, XMLFile and CSVFile) and specifically returns the actual file path including __init__.py for packages.

cammarosano self-assigned this Nov 21, 2025

fda-odoo self-requested a review November 21, 2025 17:13

fda-odoo added the enhancement New feature or request label Nov 21, 2025

fda-odoo added this to the 1.1.1 milestone Nov 21, 2025

cammarosano commented Nov 21, 2025

View reviewed changes

cammarosano force-pushed the alpha-list_namespace_dirs-rcdl branch from ce4bf39 to 9bf1d43 Compare November 21, 2025 17:28

cammarosano commented Nov 21, 2025

View reviewed changes

fda-odoo requested changes Nov 21, 2025

View reviewed changes

cammarosano commented Nov 21, 2025

View reviewed changes

cammarosano force-pushed the alpha-list_namespace_dirs-rcdl branch from 9bf1d43 to 774838c Compare November 25, 2025 12:34

cammarosano added 5 commits November 25, 2025 13:45

[IMP] list namespace dirs on hover

d2e8a97

Instead of displaying the "See also" link, we now list the namespace directories when hovering over a namespace symbol. We also make sure that the documentation section appears before the useful links/directories list section.

[REF] remove paths from RootSymbol

2a68db2

The `paths` field in `RootSymbol` has no purpose and is always an empty vector.

[REF] move add_path to NamespaceSymbol

cebf89c

Among all symbol variants, adding a path is only relevant for `NamespaceSymbol`. Therefore this commits moved the `add_path` method from the generic `Symbol` enum to the specific `NamespaceSymbol` struct.

[REF] path method in PackageSymbol

0b7874c

Package symbols only have a single path. Before this commit, the `PackageSymbol::paths` method always returned a single-element Vec<String>. This commit renames it to `path` (singular) and makes it return the single path instead.

cammarosano force-pushed the alpha-list_namespace_dirs-rcdl branch from 774838c to 77d6151 Compare November 25, 2025 12:46

		// source_file_symbol is either a file or a namespace with a single path
		let source_path = source_file_symbol.borrow().path().as_vec()[0].clone();

List namespace paths on hover + path-related refactors #484

Are you sure you want to change the base?

List namespace paths on hover + path-related refactors #484

Uh oh!

Conversation

cammarosano commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cammarosano Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cammarosano Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fda-odoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cammarosano Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cammarosano Nov 21, 2025 •

edited

Loading

cammarosano Nov 21, 2025 •

edited

Loading

cammarosano Nov 21, 2025 •

edited

Loading