categulario
/
tiempo-rs
1
0
Fork 0
Code Issues 19 Pull Requests 1 Packages Projects Releases 14 Wiki Activity

Draft: Autocomplete tests #40

Open
perrotuerto wants to merge 2 commits from dev-autocomplete into main
pull from: dev-autocomplete
merge into: categulario:main
Conversation 23 Commits 2 Files Changed 4 +85 -1
perrotuerto commented 2023-03-14 13:25:10 -06:00 (Migrated from gitlab.com)
Copy Link

I created tests for autocomplete and a new type of error (InvalidAutocompleteFormatter).
Now I wait to known all my mistakes xD

I created tests for autocomplete and a new type of error (InvalidAutocompleteFormatter). Now I wait to known all my mistakes xD
categulario commented 2023-03-14 15:01:45 -06:00 (Migrated from gitlab.com)
Copy Link

Something that we need to discuss is the matching mechanism. I think the matching must be by prefix instead of by substring. This I think creates an easier mental model for the user as to what she needs to type and what she can omit in order to invoke the desired formatter. What do you think?

Something that we need to discuss is the matching mechanism. I think the matching must be by prefix instead of by substring. This I think creates an easier mental model for the user as to what she needs to type and what she can omit in order to invoke the desired formatter. What do you think?
categulario commented 2023-03-14 15:09:04 -06:00 (Migrated from gitlab.com)
Copy Link

Secon thing is: I don't see necessary to create a new error because the current error for when a formatter is not found is quite explicit. Maybe we can extend the error message to suggest that no formatter with the given name or prefix was found.

Secon thing is: I don't see necessary to create a new error because the current error for when a formatter is not found is quite explicit. Maybe we can extend the error message to suggest that no formatter with the given name or prefix was found.
categulario commented 2023-03-14 15:52:51 -06:00 (Migrated from gitlab.com)
Copy Link

Ok, a new error is needed but for the case where the prefix is ambiguous, like if the user wrote t d -f a and she has two custom formatters: accounting and average. In this case the error should be clear enough about the ambiguity.

Ok, a new error _is_ needed but for the case where the prefix is ambiguous, like if the user wrote `t d -f a` and she has two custom formatters: `accounting` and `average`. In this case the error should be clear enough about the ambiguity.
categulario commented 2023-03-14 15:57:45 -06:00 (Migrated from gitlab.com)
Copy Link

use assert_eq! macro to test that the formatter found is the correct one

use `assert_eq!` macro to test that the formatter found is the correct one
categulario commented 2023-03-14 15:57:45 -06:00 (Migrated from gitlab.com)
Copy Link

similar to the above, use the matches! macro to check that it is the expected variant.

similar to the above, use the `matches!` macro to check that it is the expected variant.
categulario commented 2023-03-14 16:00:41 -06:00 (Migrated from gitlab.com)
Copy Link

this function will be used by production code, it should be outside of the tests submodule.

this function will be used by production code, it should be outside of the `tests` submodule.
categulario commented 2023-03-14 16:00:41 -06:00 (Migrated from gitlab.com)
Copy Link

in the end we'll probably use some macro trick to get this list from the Formatter enum itself.

in the end we'll probably use some macro trick to get this list from the Formatter enum itself.
categulario commented 2023-03-14 16:00:41 -06:00 (Migrated from gitlab.com)
Copy Link

why cannot match a formatter with only one character?

why cannot match a formatter with only one character?
perrotuerto commented 2023-03-15 11:31:37 -06:00 (Migrated from gitlab.com)
Copy Link

Do you mean FormatterNotFound Error? Because I decided to create a new one so I don't have to deal with paths and config_at xD

Do you mean `FormatterNotFound` Error? Because I decided to create a new one so I don't have to deal with `paths` and `config_at` xD
perrotuerto commented 2023-03-15 11:32:39 -06:00 (Migrated from gitlab.com)
Copy Link

Oh, ok, so I guess it won't just give the first match as it is implemented right now, but an error, right?

Oh, ok, so I guess it won't just give the first match as it is implemented right now, but an error, right?
perrotuerto commented 2023-03-15 11:34:41 -06:00 (Migrated from gitlab.com)
Copy Link

So assert_eq! or matches! or both (can it be both?)?

So `assert_eq!` or `matches!` or both (can it be both?)?
perrotuerto commented 2023-03-15 11:36:28 -06:00 (Migrated from gitlab.com)
Copy Link

To avoid ambiguity. But I remember now that you like unreadable shorthands like t -f t so I am gonna remove that constraint xD

To avoid ambiguity. But I remember now that you like unreadable shorthands like `t -f t` so I am gonna remove that constraint xD
perrotuerto commented 2023-03-15 11:43:59 -06:00 (Migrated from gitlab.com)
Copy Link

marked this merge request as draft

marked this merge request as **draft**
perrotuerto commented 2023-03-15 13:17:27 -06:00 (Migrated from gitlab.com)
Copy Link

Similar approach but I used strum for Formatter::VARIANTS instead of a macro.

Similar approach but I used [strum](https://crates.io/crates/strum) for `Formatter::VARIANTS` instead of a macro.
perrotuerto commented 2023-03-15 13:23:07 -06:00 (Migrated from gitlab.com)
Copy Link

mentioned in commit 70c9aa9355

mentioned in commit 70c9aa935575d85594540f803b3e9f4e215392dc
perrotuerto commented 2023-03-15 13:23:09 -06:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](/categulario/tiempo-rs/-/merge_requests/8/diffs?diff_id=629586411&start_sha=b41bf6dcf49f7cd7cc726e049b0f0f2775566fc7#a665ee300647643ceb022322892b853a17aa4197_98_102)
perrotuerto commented 2023-03-15 13:23:10 -06:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](/categulario/tiempo-rs/-/merge_requests/8/diffs?diff_id=629586411&start_sha=b41bf6dcf49f7cd7cc726e049b0f0f2775566fc7#a665ee300647643ceb022322892b853a17aa4197_110_109)
perrotuerto commented 2023-03-15 13:23:10 -06:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](/categulario/tiempo-rs/-/merge_requests/8/diffs?diff_id=629586411&start_sha=b41bf6dcf49f7cd7cc726e049b0f0f2775566fc7#a665ee300647643ceb022322892b853a17aa4197_83_102)
perrotuerto commented 2023-03-15 13:23:10 -06:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](/categulario/tiempo-rs/-/merge_requests/8/diffs?diff_id=629586411&start_sha=b41bf6dcf49f7cd7cc726e049b0f0f2775566fc7#a665ee300647643ceb022322892b853a17aa4197_84_102)
perrotuerto commented 2023-03-15 13:23:11 -06:00 (Migrated from gitlab.com)
Copy Link

changed this line in version 2 of the diff

changed this line in [version 2 of the diff](/categulario/tiempo-rs/-/merge_requests/8/diffs?diff_id=629586411&start_sha=b41bf6dcf49f7cd7cc726e049b0f0f2775566fc7#a665ee300647643ceb022322892b853a17aa4197_85_102)
perrotuerto commented 2023-03-15 13:23:11 -06:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

Compare with previous version

added 1 commit <ul><li>70c9aa93 - Fix some issues from !8</li></ul> [Compare with previous version](/categulario/tiempo-rs/-/merge_requests/8/diffs?diff_id=629586411&start_sha=b41bf6dcf49f7cd7cc726e049b0f0f2775566fc7)
categulario commented 2023-03-15 16:05:18 -06:00 (Migrated from gitlab.com)
Copy Link

this function doesn't access &self or variants and it already belongs to an appropriate namespace, therefore it can live outside of any impl block just by itself.

this function doesn't access `&self` or variants and it already belongs to an appropriate namespace, therefore it can live outside of any impl block just by itself.
perrotuerto commented 2023-03-15 19:09:56 -06:00 (Migrated from gitlab.com)
Copy Link

Ok, I can do that. But this makes me think if it is better idea to refactor from_str so it tries to do before the autocomplete and, therefore, there is no autocomplete fn needed. The main diff is that in case that there is no match during the autocomplete attempt, &str will the treated as a custom formatter, instead of throwing an error. What do you think?

Ok, I can do that. But this makes me think if it is better idea to refactor `from_str` so it tries to do before the autocomplete and, therefore, there is no `autocomplete` fn needed. The main diff is that in case that there is no match during the autocomplete attempt, `&str` will the treated as a custom formatter, instead of throwing an error. What do you think?
This pull request has changes conflicting with the target branch.
  • src/formatters.rs
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b dev-autocomplete main
git pull origin dev-autocomplete

Step 2:

Merge the changes and update on Forgejo.
git checkout main
git merge --no-ff dev-autocomplete
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
config

   
feature

   
formatters

   
idea

   
new-command
No Label
bug
config
feature
formatters
idea
new-command
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: categulario/tiempo-rs#40
No description provided.
Delete Branch "dev-autocomplete"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?