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.

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

FIXED TODO: Avoided take() function on the run editor method #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rrsmart8
Copy link

@rrsmart8 rrsmart8 commented Jan 5, 2024

pub fn run(&mut self, file_name: &Option<String>) -> Result<(), Error> {
    // File handling logic
    if let Some(path) = file_name.as_ref().and_then(|p| Some(sys::path(p.as_str()))) {
        self.select_syntax_highlight(path.as_path())?;
        self.load(path.as_path())?;
        self.file_name = Some(path.to_string_lossy().to_string());
    } else {
        self.rows.push(Row::new(Vec::new()));
        self.file_name = None;
    }

    // Main loop
    loop {
        // Display mode status if present
        if let Some(mode) = self.prompt_mode.as_ref() {
            set_status!(self, "{}", mode.status_msg());
        }

        // Refresh screen, get user input
        self.refresh_screen()?;
        let key = self.loop_until_keypress()?;

        // Process keypress without using take() on self.prompt_mode
        self.prompt_mode = match self.prompt_mode.map_or_else(|| None, |pm| Some(pm.process_keypress(self, &key)?)) {
            None => match self.process_keypress(&key) {
                (true, _) => return Ok(()),
                (false, new_mode) => Some(new_mode),
            },
            pm => pm,
        };
    }
}

rrsmart8

This comment was marked as resolved.

Copy link
Author

@rrsmart8 rrsmart8 left a comment

Choose a reason for hiding this comment

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

Handled the case where the file already exists when the user tries to save_as

Copy link

@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

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

Please describe your changes in the pull request description.

@@ -508,6 +508,13 @@ impl Editor {
/// attribute of the editor will be set and syntax highlighting will be updated.
fn save_as(&mut self, file_name: String) -> Result<(), Error> {
// TODO: What if file_name already exists?

Choose a reason for hiding this comment

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

Remove the TODO comment.

self.save_and_handle_io_errors(&file_name);
self.file_name = Some(file_name);
Key::Char(SAVE) => {
match mem::replace(&mut self.file_name, None) {

Choose a reason for hiding this comment

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

This has the same effect as take, I would leave the take function in place has it adds more meaning. Your solution also adds an extra line of code.

Choose a reason for hiding this comment

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

I think you could avoid using take by borrowing the value in the match arm:

Key::Char(SAVE) => match self.file_name {
                // TODO: Can we avoid using take() then reassigning the value to file_name?
                Some(ref file_name) => {
                    self.save_and_handle_io_errors(file_name);
                    self.file_name = Some(file_name);
                // ...

@@ -691,31 +699,33 @@ impl Editor {
///
/// Will Return `Err` if any error occur.
pub fn run(&mut self, file_name: &Option<String>) -> Result<(), Error> {
if let Some(path) = file_name.as_ref().map(|p| sys::path(p.as_str())) {
if let Some(path) = file_name.as_ref().and_then(|p| Some(sys::path(p.as_str()))) {

Choose a reason for hiding this comment

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

Why use map_or_else?

self.prompt_mode = match self.prompt_mode.take() {
// process_keypress returns (should_quit, prompt_mode)

self.prompt_mode = match self.prompt_mode.map_or_else(|| None, |pm| Some(pm.process_keypress(self, &key)?)) {

Choose a reason for hiding this comment

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

Why use map_or_else?

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