Implemented OrderSelect #16
Conversation
|
For the testing i removed the |
|
The implementation is now completed |
There was a problem hiding this comment.
Rather than the filter and validate working on only the indices, I think it makes sense to implement AsRef<str> for ListItem and use a ChoiceList<Text<ListItem>>. This way if the validator or filterer requires the string associated with each option, they can access that as well. This would require many changes throughout, as it collapses order and choices to one member.
Also add integration tests
|
Yeah, there is a ton of things to change. I'll do that in the next days. |
|
I'm not really sure the use of What's the advantage of using |
|
|
|
Ok i'll stick with |
|
And also since it's a list of The only option i see here is to not use the |
|
Yeah, getting a decent API surface is difficult here because of the internals, but I think I have a found a good solution. I'm going to push a change that converts /// The representation of each choice in an [`OrderSelect`].
///
/// It is different from [`ListItem`](crate::answer::ListItem) due to an implementation detail.
#[derive(Debug)]
pub struct OrderSelectItem {
index: usize,
text: Text<String>,
}
impl OrderSelectItem {
/// The index of the choice
pub fn index(&self) -> usize {
self.index
}
/// The content of the choice -- it is what is displayed to the user
pub fn text(&self) -> &str {
&self.text.text
}
}
impl Widget for OrderSelectItem {
fn render<B: Backend>(
&mut self,
layout: &mut ui::layout::Layout,
backend: &mut B,
) -> io::Result<()> {
self.text.render(layout, backend)
}
fn height(&mut self, layout: &mut ui::layout::Layout) -> u16 {
self.text.height(layout)
}
fn cursor_pos(&mut self, layout: ui::layout::Layout) -> (u16, u16) {
self.text.cursor_pos(layout)
}
fn handle_key(&mut self, key: KeyEvent) -> bool {
self.text.handle_key(key)
}
}Then, instead of using
pub(crate) fn new(name: String) -> Self {
Self {
opts: Options::new(name),
order_select: SelectList::new(|_| true),
}
}After making these changes, validate can take |
|
Alright i'll implement this right now |
|
By researching, i found that changing type back to From a performance perspective i don't know if this is an issue or not |
|
Maybe replacing Since Or otherwise we left as it is, after all they behave the same way here |
|
Or we can implement a Trait for this, so we can keep this consistency What do you think is the best option? |
They are not caches. When you run the test, it checks that the result produced matches the snapshot that already exists. The exception is when you run the tests as To see more about snapshot testing, you can read the documentation of the
Yes, I really need to write some documentation about the internal workings of the library.
The
This would be great, after all |
src/question/order_select/builder.rs
Outdated
| /// NOTE: The boolean [`slice`] contains a boolean value for each index even if it is a | ||
| /// separator. However it is guaranteed that all the separator indices will be false. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use requestty::Question; | ||
| /// | ||
| /// let order_select = Question::order_select("home_tasks") | ||
| /// //... | ||
| /// .validate(|tasks, previous_answers| { | ||
| /// if tasks[0].text() == "Make the bed" { | ||
| /// Err("You have to make the bed first".to_string()) | ||
| /// } else { | ||
| /// Ok(()) | ||
| /// } | ||
| /// }) | ||
| /// //... | ||
| /// .build(); | ||
| /// ``` |
There was a problem hiding this comment.
Remove the "NOTE", and like the tests above, the then and else block are swapped. The condition is checking that the first one is "Make the bed", but if it is, it is returning an error that the first one should be to make the bed.
|
What i was meaning was that instead of storing the snapshots on this repository, the developer can build the snapshots themselves before performing tests That's why i suggested to add a category in README.md or another file, so the future developers know how to perform tests correctly |
|
Also here how i'll implement back
What do you think about this way ? |
|
I feel like I didn't convey my point across properly. I do not want to change the types for #[derive(Debug)]
pub(super) struct OrderSelect<'a> {
choices: SelectList<OrderSelectItem>,
max_index_width: usize,
moving: bool,
transform: Transform<'a, [ListItem]>, // Only thing that changes
validate: Validate<'a, [OrderSelectItem]>,
filter: Filter<'a, Vec<OrderSelectItem>>,
}And the impl Prompt for OrderSelectPrompt<'_, '_> {
// ...
type Output = Vec<ListItem>;
// ...
fn finish(self) -> Self::Output {
// ...
c.into_iter()
.map(From::from)
.collect()
}
// ...
}And the |
The way snapshot tests work is as following: When you first write the test, you run Then you run Only after all of this is done, can the tests be run how they usually are. When running a test which contains I urge you to also read the content of the links here. I hope you understand why the snapshots need to be stored in the repository. If you have any questions, please feel free to ask.
Yes, I agree with adding documentation for how to run the tests correctly. This is definitely needed, but I will get to it after some time. |
|
Maybe what you're trying to say here is that, in my way, the developer needs to accept each snapshot one after another, which can be tedious But there is a command to accept them all at once : That's why is was suggesting to remove this folders, because the snapshots can be reconstructed with this series of command:
We can keep them if you want, it was just an important suggestion after all |
|
By the way, the |
|
Also for the implementation you suggested, i'm not very comfortable with the fact that there is different types (and so different ways to access the data) for each functions of this builder, this can confuse the user Yes, this can add consistency with the Personally i would prefer to keep the same type (and so the same ways to access the data) for each function of the same builder here, to keep the consistency in the builder itself. I hope you understand my point of view on this subject |
|
I understand where you are coming from. The reason I suggested the change was that maintaining compatibility with However, the point you raise is also valid. I leave the final decision of whether to change the type for |
Yes, the Since you are running windows, don't worry about the |
Yes, Take the following example. Let's say that through some change for a different purpose, accidentally in Currently as it is the snapshots are stored. They are checked to be correct when they are created and only then accepted. Then if the rendering changes by accident, the index in the test would not match the one in the snapshot, and so the test would fail. If snapshots were not stored, and the 3 commands above were run, the snapshots created would include the wrong index as
We will keep them, but I feel it is important that you understand why we need to keep them |
|
Since the only difference between This way both of our requirements are satisfied, you have the Also The only con is that we need to access the |
|
I prefer to keep The reason that |
|
Ok so OrderSelect should be ready now |
There was a problem hiding this comment.
Most of it looks good. There are a couple of small changes here that need to be made. Other than this, the requestty-macro crate needs to be changed to allow for OrderSelect, the gifs need to be added and the snapshots need to be added. I will push a commit containing all of these changes, after which I will enable the CI for this PR. Please fix any errors that come with the CI
src/question/order_select/mod.rs
Outdated
| impl Into<ListItem> for OrderSelectItem { | ||
| fn into(self) -> ListItem { | ||
| ListItem { index: self.initial_index, text: self.text.text } | ||
| } | ||
| } |
There was a problem hiding this comment.
It is better to implement From<OrderSelectItem> for ListItem instead. See here.
tests/order_select.rs
Outdated
| } | ||
| b.set_fg(ui::style::Color::Reset) | ||
| }) | ||
| .message("multi select") |
There was a problem hiding this comment.
This is still multi-select
|
Also now i understand for the snapshots folder, thanks for the explanation |
|
Congrats, this PR has been merged! |
Here is a first beta of the order_select question (Discussed in [#13]), but i'ts not complete.
I still have to :
I intentionally not implemented many functions in the builder (choice, choice_with_default, separator, default_spearator, choice_with_default) for the following reasons :
Let me know if you want changes in the code.
Only tested on Windows 11.