zeroc-ice / vscode-slice

Slice syntax highlighter for Visual Studio Code
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Refactor `resolve_reference_paths` #15

Closed InsertCreativityHere closed 10 months ago

InsertCreativityHere commented 10 months ago

This PR refactors the stated function. No logic of any kind should be modified by this PR, it just reorganizes the code and adds some explanatory comments for readability.

Many of the function chains included repetitive checks for whether an option was Some, or some conversion succeeded. Instead of doing these over and over, they were transformed into let-else guards we do at the top of the function.

There's 2 places I suspect could be further improved, but would be altering logic:

We have this line: root_path.join(&root_path).display().to_string() which joins the root_uri to itself. That's either no-op or totally broken right? Or is this necessary to fix some weird pathing problem.

In the original code, this happened when self.references = None. So try_get_reference_urls returns root_uri as a default. But the code in resolve_reference_paths ALWAYS calls root_path.join(path), even when path was defaulted to root_path. It was impossible to see this oddity before refactoring.


In some parts of the server, we seem to rely on root_uri being an absolute path, but in other places (this function) we guard against it being relative. Do we have any guarantees about this? Either it's guaranteed and these checks are dead code, or we should always be checking if it's relative.


Also, try_get_reference_urls used to return some helpful error messages, but then the function that calls it just always did unwrap_or_default(), throwing these errors away. It's not clear to me that those cases were actually errors, or just some config wasn't set or something (which is valid to do).

CLAassistant commented 10 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Austin Henriksen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.