-
Notifications
You must be signed in to change notification settings - Fork 41
chore: remove configFile and state.json #60
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
Conversation
src/state.ts
Outdated
return false; | ||
} | ||
} | ||
connectionString?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove this - since it's no longer persisted, it is redundant.
@@ -1,34 +1,8 @@ | |||
import { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver"; | |||
import { AsyncEntry } from "@napi-rs/keyring"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove this package dependency if we're not going to use it.
src/config.ts
Outdated
localDataPath, | ||
configPath, | ||
}; | ||
fs.mkdirSync(logPath, { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated to the refactor but we should handle this call failing as we might not have permissions etc. and this is outside our try/catch
Looks like this is now conflicting with the integration tests. You can remove the mocks from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be better for the apiClient to have a state rather than for a state to have an apiClient? I could explore this in #55
same principle for the connection provider of MongoDB tools, seems like this state is a session rather than state |
simplifying for now, we should be able to add them later