Skip to content

Conversation

InsertCreativityHere
Copy link
Member

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
Copy link

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.

@InsertCreativityHere InsertCreativityHere merged commit 67193f2 into zeroc-ice:main Nov 29, 2023
ReeceHumphreys pushed a commit that referenced this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants