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.

Sign up for GitHub

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

Jump to bottom

feat: add suggestions #74

Merged
merged 18 commits into from
Mar 5, 2024
Merged

feat: add suggestions #74

merged 18 commits into from
Mar 5, 2024

Conversation

bashbunni
Copy link
Member

builds off of the delete-db PR. Suggests a db name that does exist based on the first two characters of a mistyped arg. Just kept it simple, but would be great to hear if there are better ways to do this. I also experimented with having a more reusable function for getting the []os.DirEntry for a default client.

Would also love some guidance on how I could write tests for this 💭

@aymanbagabas aymanbagabas changed the base branch from main to delete-db May 30, 2023 16:14
caarlos0
caarlos0 reviewed May 30, 2023
View reviewed changes
aymanbagabas
aymanbagabas reviewed May 30, 2023
View reviewed changes
Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

Few comments

caarlos0
caarlos0 reviewed May 30, 2023
View reviewed changes
caarlos0
caarlos0 reviewed May 30, 2023
View reviewed changes
@bashbunni bashbunni changed the title Feat/suggestions feat: add suggestions May 31, 2023
bashbunni
bashbunni commented May 31, 2023
View reviewed changes
@bashbunni
Copy link
Member Author

Hmmm tests are making CI fail with /home/runner/.local/share/charm/cloud.charm.sh/kv: no such file or directory.

Open to suggestions on how to make it more stable :)

@bashbunni bashbunni changed the base branch from delete-db to main May 31, 2023 21:47
caarlos0
caarlos0 reviewed Jun 1, 2023
View reviewed changes
main.go Outdated
Comment on lines 127 to 131
type suggestionNotFoundErr struct {
suggestions []string
}

func (e suggestionNotFoundErr) Error() string {
Copy link
Member

Choose a reason for hiding this comment

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

the name makes me think this error happens when no suggestions are found, but seems like its not it... maybe another name?

💅: I think prefixing the error with err is more common than suffixing with it

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm very good point. I had it as dbNotFoundErr before which I think makes more sense. Also will change e -> err

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait or did you mean that it's more common to see errDbNotFound vs dbNotFoundErr?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, yes, at least in the stdlibrary... eg. os.ErrNotExists and many others... although all of those are public, haven't checked the private ones 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Just did a quick check - the private ones also prefix with err instead of having it as a suffix. Thanks for the tip! It's a great call to use stdlib for conventions in your own code 💭

caarlos0
caarlos0 reviewed Jun 1, 2023
View reviewed changes
@bashbunni
Copy link
Member Author

Yeah, so my tests are making the Ubuntu build fail... Going to need to change the path to a temp dir with an env variable (CHARM_DATA_DIR=<path-to-temp-dir>) for tests to pass? Gonna check it out

caarlos0
caarlos0 reviewed Jun 8, 2023
View reviewed changes
@caarlos0
Copy link
Member

caarlos0 commented Jun 8, 2023

noice!

@bashbunni
Copy link
Member Author

Not sure if I should also add a check for reflect.DeepEqual on the suggestions we get back 🤔. Not sure that ordering is consistent... Will check if it changes anything locally, so don't merge yet pls 😄

@bashbunni
Copy link
Member Author

oookay I improved the tests and added a formatting func for the databases which I needed for the tests and hopefully makes formatting dbs more reusable. It adds an extra loop for our getDbs function though which is not ideal because I want the formatting func to take a []string as arg

@bashbunni bashbunni requested a review from aymanbagabas June 9, 2023 19:24
@bashbunni bashbunni linked an issue Jun 12, 2023 that may be closed by this pull request
feat: provide user feedback when db name doesn't exist #72
Closed
@bashbunni
Copy link
Member Author

bashbunni commented Mar 5, 2024 •
edited
Loading

Will update the crypto package (charm is the direct dep) in another PR to fix the CI errors

@bashbunni bashbunni merged commit 1aaa25c into main Mar 5, 2024
10 of 12 checks passed
@bashbunni bashbunni deleted the feat/suggestions branch March 5, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caarlos0 caarlos0 caarlos0 approved these changes

@aymanbagabas aymanbagabas aymanbagabas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: provide user feedback when db name doesn't exist
3 participants
@bashbunni @caarlos0 @aymanbagabas