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

Store a ShellConfig in FaunaCommand #252

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Conversation

macmv
Copy link
Contributor

@macmv macmv commented Sep 18, 2023

Ticket(s): FE-###

Stores a ShellConfig in FaunaCommand, which will let us use the config in all the commands that need it.

@macmv macmv requested a review from fauna-chase September 18, 2023 22:21
@macmv macmv force-pushed the store-config-in-fauna-command branch from e346896 to 8102739 Compare September 18, 2023 23:06
@macmv macmv force-pushed the remove-args-from-shell-config branch from c2a0ae6 to 2e13d99 Compare September 25, 2023 17:40
@macmv macmv force-pushed the store-config-in-fauna-command branch from 61d26da to 297436b Compare September 25, 2023 17:42
Comment on lines +222 to +229
validate = () => {
if (this.endpoint === undefined) {
// No `~/.fauna-shell` was found, and no `--secret` was passed.
throw new Error(
"No endpoint or secret set. Set an endpoint in ~/.fauna-shell, .fauna-project, or pass --endpoint"
);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

we do almost all of the validation in the constructor for this class and then just do the endpoint in the validate method. It seems that we are doing it this way because some commands don't require an endpoint. Which ones are those?
Given that it seems we are saying that this class is technically 'valid' without an endpoint. With that, I think I'd rather have the code paths that need it ensure there is an endpoint without this method on this class.

Copy link
Contributor Author

@macmv macmv Sep 27, 2023

Choose a reason for hiding this comment

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

yup, this should be renamed and refactored a bit to move more validation into here. i'll do that in a future PR

Copy link
Contributor

@fauna-chase fauna-chase left a comment

Choose a reason for hiding this comment

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

other than my one minor comment, LGTM

Base automatically changed from remove-args-from-shell-config to main September 27, 2023 20:49
@macmv macmv force-pushed the store-config-in-fauna-command branch from 297436b to 5ad39e4 Compare September 27, 2023 20:51
@macmv macmv merged commit 34ab4d9 into main Sep 27, 2023
@macmv macmv deleted the store-config-in-fauna-command branch September 27, 2023 20:51
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