From 960024dffb0474658eb98e5d5bcbfe8573ce06b6 Mon Sep 17 00:00:00 2001
From: Josef Reidinger <jreidinger@suse.cz>
Date: Mon, 17 Jul 2023 10:54:19 +0200
Subject: [PATCH] improve documentation

---
 rust/agama-dbus-server/src/questions.rs | 32 +++++++++++++++++++++++--
 rust/agama-lib/src/questions.rs         | 19 +++++++++++----
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/rust/agama-dbus-server/src/questions.rs b/rust/agama-dbus-server/src/questions.rs
index e8f7404f90..e9acab9e18 100644
--- a/rust/agama-dbus-server/src/questions.rs
+++ b/rust/agama-dbus-server/src/questions.rs
@@ -6,7 +6,7 @@ use agama_lib::{
     questions::{self, GenericQuestion, WithPassword},
 };
 use anyhow::Context;
-use log::{self, info};
+use log;
 use zbus::{dbus_interface, fdo::ObjectManager, zvariant::ObjectPath, Connection};
 
 #[derive(Clone, Debug)]
@@ -80,13 +80,27 @@ enum QuestionType {
     BaseWithPassword,
 }
 
+/// Trait for objects that can provide answers to all kind of Question.
 trait AnswerStrategy {
     /// Provides answer for generic question
+    ///
+    /// I gets as argument the question to answer. Returned value is `answer`
+    /// property or None. If `None` is used, it means that this object does not
+    /// answer to given question.
     fn answer(&self, question: &GenericQuestion) -> Option<String>;
     /// Provides answer and password for base question with password
+    ///
+    /// I gets as argument the question to answer. Returned value is pair
+    /// of `answer` and `password` properties. If `None` is used in any
+    /// position it means that this object does not respond to given property.
+    ///
+    /// It is object responsibility to provide correct pair. For example if
+    /// possible answer can be "Ok" and "Cancel". Then for `Ok` password value 
+    /// should be provided and for `Cancel` it can be `None`.
     fn answer_with_password(&self, question: &WithPassword) -> (Option<String>, Option<String>);
 }
 
+/// AnswerStrategy that provides as answer the default option.
 struct DefaultAnswers;
 
 impl AnswerStrategy for DefaultAnswers {
@@ -118,6 +132,7 @@ impl Questions {
         default_option: &str,
         data: HashMap<String, String>,
     ) -> Result<ObjectPath, zbus::fdo::Error> {
+        log::info!("Creating new question with text: {}.", text);
         let id = self.last_id;
         self.last_id += 1; // TODO use some thread safety
         let options = options.iter().map(|o| o.to_string()).collect();
@@ -150,6 +165,7 @@ impl Questions {
         default_option: &str,
         data: HashMap<String, String>,
     ) -> Result<ObjectPath, zbus::fdo::Error> {
+        log::info!("Creating new question with password with text: {}.", text);
         let id = self.last_id;
         self.last_id += 1; // TODO use some thread safety
                            // TODO: share code better
@@ -232,16 +248,27 @@ impl Questions {
     }
 
     /// tries to provide answer to question using answer strategies
+    /// 
+    /// What happens underhood is that it user answer_strategies vector
+    /// and try to find the first strategy that provides answer. When
+    /// aswer is provided, it returns immediatelly.
     fn fill_answer(&self, question: &mut GenericQuestion) -> () {
         for strategy in self.answer_strategies.iter() {
             match strategy.answer(question) {
                 None => (),
-                Some(answer) => question.answer = answer,
+                Some(answer) => {
+                    question.answer = answer;
+                    return ()
+                }
             }
         }
     }
 
     /// tries to provide answer to question using answer strategies
+    /// 
+    /// What happens underhood is that it user answer_strategies vector
+    /// and try to find the first strategy that provides answer. When
+    /// aswer is provided, it returns immediatelly.
     fn fill_answer_with_password(&self, question: &mut WithPassword) -> () {
         for strategy in self.answer_strategies.iter() {
             let (answer, password) = strategy.answer_with_password(question);
@@ -250,6 +277,7 @@ impl Questions {
             }
             if let Some(answer) = answer {
                 question.base.answer = answer;
+                return ()
             }
         }
     }
diff --git a/rust/agama-lib/src/questions.rs b/rust/agama-lib/src/questions.rs
index 2737305381..61f9894f22 100644
--- a/rust/agama-lib/src/questions.rs
+++ b/rust/agama-lib/src/questions.rs
@@ -1,4 +1,3 @@
-use log::info;
 use std::collections::HashMap;
 
 /// module holdings data model for agama questions
@@ -33,7 +32,6 @@ impl GenericQuestion {
         default_option: String,
         data: HashMap<String, String>,
     ) -> Self {
-        info!("Creating new generic question with text {}", text);
         Self {
             id,
             class,
@@ -51,7 +49,21 @@ impl GenericQuestion {
 }
 
 /// Composition for questions which include password.
-/// TODO: research a bit how ideally do mixins in rust
+///
+/// ## Extension
+/// If there is need to provide more mixins, then this structure does not work
+/// well as it is hard do various combinations. Idea is when need for more
+/// mixins arise to convert it to Question Struct that have optional mixins
+/// inside like
+/// 
+/// struct Question {
+///   base: GenericQuestion,
+///   with_password: Option<WithPassword>,
+///   another_mixin: Option<AnotherMixin>
+/// }
+/// 
+/// This way all handling code can check if given mixin is used and
+/// act appropriate.
 #[derive(Clone, Debug)]
 pub struct WithPassword {
     /// Luks password. Empty means no password set.
@@ -62,7 +74,6 @@ pub struct WithPassword {
 
 impl WithPassword {
     pub fn new(base: GenericQuestion) -> Self {
-        info!("Adding to question with password interface.");
         Self {
             password: "".to_string(),
             base,