Skip to content
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

fix: fallback to no type checking cache when db file can't be created #15180

Merged
merged 4 commits into from
Jul 13, 2022

Conversation

dsherret
Copy link
Member

Resolves a todo I forgot to resolve in #15118.

@dsherret dsherret requested a review from kitsonk July 13, 2022 01:57
match Self::try_new(db_file_path) {
Ok(cache) => cache,
Err(err) => {
log::debug!("Creating the type checking cache failed.\n{:#}", err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: warn that type checking is disabled if it was specified as enabled somewhere? Clearly we are changing the behaviour and we shouldn't do that silently.

Copy link
Member Author

@dsherret dsherret Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type checking isn't disabled in this case. When this fails it will always type check instead of potentially having a performance improvement and skipping type checking. So it's not really a behaviour change, but just a savings that someone won't have when it fails. Perhaps it would still be worthwhile to warn?

(I'm going to update this to try again to create the db when it fails to open)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it does warn, right? The title just confused me in the sense I stopped reading the title I skipped the "cache" word. Any warning that tells them any action to take makes sense to me, but it is internal "performance" versus what I assumed what a change in behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it won't warn and will just show this message only when debug logging is enabled. I was thinking we could tell people to run with debug logging in case they find the type checking cache never works for them so we could get more details. I'm not sure if this will be a widespread problem.

Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with optional suggestion on error message

cli/cache/check.rs Outdated Show resolved Hide resolved
dsherret and others added 2 commits July 12, 2022 22:16
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@dsherret dsherret merged commit 698eeb9 into denoland:main Jul 13, 2022
@dsherret dsherret deleted the more_robust_type_checking_cache branch July 13, 2022 02:44
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.

2 participants