Skip to content

codeReview#1

Open
AdamSedla wants to merge 149 commits into
dummyfrom
main
Open

codeReview#1
AdamSedla wants to merge 149 commits into
dummyfrom
main

Conversation

@AdamSedla

Copy link
Copy Markdown
Owner

No description provided.

Comment thread frontend/index.html Outdated
Comment thread frontend/index.html Outdated
Comment thread frontend/index.html Outdated
Comment thread .gitignore
Comment thread frontend/index.html Outdated
Comment thread frontend/style.css Outdated
Comment thread frontend/style.css Outdated
Comment thread frontend/style.css Outdated
Comment thread frontend/style.css Outdated
Comment thread frontend/style.css Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread backend/src/mail_sender.rs Outdated
Comment thread src-tauri/Cargo.lock Outdated
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa"

[[package]]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tenhle commit neni v imperativu :D

//only for display purpose
let app_folder_path_string = app_folder_path.to_str().unwrap_or("");

let empty_config: String = format!(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Máš po ruce serde, tohle by mělo bejt:

  1. vytvoření Default instance
  2. modifikace tý instance (nastavíš app_folder_path -- tvoje současný řešení neni úplně ideální, protože jak se to serializuje když app_folder_path_string obsahuje uvozovky? :D nevalidní config)
  3. serde ron serializace tý instance

Tohle řešení je taky fragile, protože si musíš vzpomenout ho updatovat když přidáš nový fieldy

)"
);

std::fs::write(&config_file_path, empty_config.clone())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ten .clone je skoro určitě zbytečnej, mělo by stačit &empty_config

);

std::fs::write(&config_file_path, empty_config.clone())
.unwrap_or_else(|_| error_of_fail_back_system(app));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fail_back -> fallback?

}

pub fn create_empty_config(app: AppHandle) -> PathBuf {
let app_folder_path = create_app_folder_path(app.clone());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

app.clone() je podezřelý, jestli to nemutuješ, tak by imo mělo stačit &app a aby create_app_folder() bral &AppHandle

});
}

pub fn export_other_mail_list(&mut self) -> Vec<Person> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tohle se jmenuje export_.... ale v podstatě to dělá jenom jiný view na data, klidně by se to mohlo jmenovat třeba recipients()

Comment thread src-tauri/Cargo.toml
[lib]
# The `_lib` suffix may seem redundant but it is necessary
# to make the lib name unique and wouldn't conflict with the bin name.
# This seems to be only an issue on Windows, see https://github.com/rust-lang/cargo/issues/8519

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wtf zkurvený widle

pub fn sender_name(&self) -> &str {
&self.sender_name
}
pub fn save_sender_name(&mut self, text: String) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tyhle metody by všechny měly bejt set_XXX místo save_XXX, save implikuje že to ukladas na dosk

pub fn error_pick_file(app: tauri::AppHandle) {
let error_message: String = "Došlo k chybě při výběru souboru.".to_string();

let _ = send_error_mail(error_message, app.clone());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

error_message by neměl bejt vynucenej String. Pokud je interně potřeba aby to byl String, tak si to konvertuj na začátku send_error_mail

Comment on lines +212 to +214
pub fn file_is_valid(&self) -> bool {
self.files.is_some()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

file vs files?

Comment on lines +58 to +61
hx-post="command:close_settings_password"
hx-trigger="click"
hx-target="#overlay-password"
hx-swap="outerHTML"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

chybí jedna úroveň odsazení, aby bylo vidět že to jsou atributy k buttonu

Comment on lines +66 to +67
placeholder="Heslo"
name="text"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

přidat odsazení, same

Comment on lines +110 to +111
app_state.mail.lock().clear();
app_state.other_mail_list.lock().clear();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Proč otvírání settings clearuje mail listy? 🤔

let mail_list = app_state.mail_list.lock();

let markup: Markup = html! {
@for i in 0..24 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

odkud se bere magické číslo 24? buď by to měl bejt výraz kterej se odvozuje např. od dýlky nějakýho fieldu na mail_listu, nebo zadefinovaná konstanta

let mail_list = app_state.mail_list.lock();

let markup: Markup = html! {
@for i in 24..29 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

další suspicious magická čísla

};

if file_picker_text.len() > 28 {
let shortened_text: String = file_picker_text.chars().take(27).collect();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

proč 28? magický číslo :D

Comment on lines +247 to +252
let path_string = path.first().expect("path should exist").to_string();

let file_name = path_string
.split_terminator("/")
.last()
.expect("file name should exist");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Path ti umí vyextrahovat filename, tohle je zbytečný, myslim že ta metoda je doslova .filename() :D

Comment thread src-tauri/src/lib.rs
Comment on lines +12 to +18
mod backend {
pub mod config;
pub mod error_handling;
pub mod mail_list_utils;
pub mod mail_sender;
pub mod other_mail_utils;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tohle typicky děláš jako backend.rs nebo backend/mod.rs spíš než inline module

Comment thread src-tauri/src/lib.rs
mail_list: Mutex<MailList>,
other_mail_list: Mutex<OtherMailList>,
settings_current_person_id: Mutex<Option<usize>>,
config: Mutex<Config>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

config má bejt editovatelnej z vnitřku aplikace?

Comment thread src/htmx.js
@@ -0,0 +1,5261 @@
var htmx = (function() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tenhle soubor neni tvůj zdroják, nepatří pod src/ ---- lepší nápad by byl to mít buď v nějaký static/assets složce nebo importovat HTMX přes CDN.

HTMX je tvoje závislost, když to pojmenuješ prostě htmx.js, víš na jaký verzi HTMX závisíš?

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