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

feat(#9): storage init #20

Merged
merged 18 commits into from
Jun 5, 2024
Merged

feat(#9): storage init #20

merged 18 commits into from
Jun 5, 2024

Conversation

h1alexbel
Copy link
Owner

@h1alexbel h1alexbel commented Jun 5, 2024

@l3r8yJ take a look, please
ref #9


PR-Codex overview

The focus of this PR is adding XML storage functionality with tests and updating dependencies.

Detailed summary

  • Added anyhow, log, env_logger, and tempdir dependencies
  • Implemented XML storage functionality in xml module
  • Added tests for XML storage creation with different file names

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@h1alexbel
Copy link
Owner Author

@l3r8yJ I suggest the following test convention: all unit test we specify in the same .rs file, while integration tests will go to the separate folder called /tests. WDYT?

@h1alexbel h1alexbel requested a review from l3r8yJ June 5, 2024 09:28
Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel take a look, please

pub fn init() {
let path = "fakehub.xml";
info!("Initializing XML storage: {path}");
File::create(path).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it would be better to handle result of creation File::create(path) and give an human readable message in error case

Copy link
Owner Author

@h1alexbel h1alexbel Jun 5, 2024

Choose a reason for hiding this comment

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

@l3r8yJ did you meant foo.expect("some message after failure")?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@h1alexbel i meant smth like

let creating_file = File::create("storage.xml");
let storage = match creating_file {
    Ok(file) => file,
    Err(error) => panic!("Problem during creating the file: {:?}", error),
};

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah, got it, thanks

src/xml/storage.rs Outdated Show resolved Hide resolved
src/xml/storage.rs Outdated Show resolved Hide resolved
src/xml/storage.rs Outdated Show resolved Hide resolved
@h1alexbel h1alexbel requested a review from l3r8yJ June 5, 2024 10:48
Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel last comment, take a look, please

let path = "fakehub.xml";
info!("Initializing XML storage: {path}");
let creating = File::create(path);
let _ = match creating {
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be we should return the file from touch_storage and pass it to next function instead of sharing it by name?

@h1alexbel h1alexbel requested a review from l3r8yJ June 5, 2024 11:10
Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel a few changes

use crate::xml::storage::touch_storage;

fn clean() {
fs::remove_file("fakehub.xml").unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think better solution would be using temp files, like this

Copy link
Owner Author

Choose a reason for hiding this comment

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

@l3r8yJ we create this file inside touch_storage(), how we can create a temp file there? Or I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@h1alexbel you can pass a temp directory and create file inside of it. I think that even better would be if touch_storage will accept directory or smth like that

}

#[test]
fn creates_xml_storage() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

best practise is to return anyhow::Result<()> and add Ok(()) at the end of test

src/xml/storage.rs Outdated Show resolved Hide resolved
Co-authored-by: Ivan Ivanchuk <[email protected]>
@l3r8yJ
Copy link
Collaborator

l3r8yJ commented Jun 5, 2024

@l3r8yJ I suggest the following test convention: all unit test we specify in the same .rs file, while integration tests will go to the separate folder called /tests. WDYT?

@h1alexbel yes, good idea

@l3r8yJ
Copy link
Collaborator

l3r8yJ commented Jun 5, 2024

@h1alexbel do you mind if I pull your branch and make some changes to it?

@h1alexbel
Copy link
Owner Author

@l3r8yJ no, but I already applied the temp dir changes

@h1alexbel h1alexbel requested a review from l3r8yJ June 5, 2024 13:12
Copy link
Collaborator

@l3r8yJ l3r8yJ left a comment

Choose a reason for hiding this comment

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

@h1alexbel looks good to me! added some fix

@h1alexbel
Copy link
Owner Author

h1alexbel commented Jun 5, 2024

@l3r8yJ looks like @rultor is alive, let's try to merge with his help

@h1alexbel
Copy link
Owner Author

h1alexbel commented Jun 5, 2024

@rultor merge

@h1alexbel
Copy link
Owner Author

@rultor is broken again

@h1alexbel h1alexbel merged commit d7e2372 into master Jun 5, 2024
11 checks passed
@h1alexbel h1alexbel deleted the xml branch June 5, 2024 14:04
@rultor
Copy link
Collaborator

rultor commented Jun 5, 2024

@rultor merge

@h1alexbel The pull request is closed already, so I can't merge it

@rultor
Copy link
Collaborator

rultor commented Jun 5, 2024

@rultor merge

@h1alexbel I'm sorry, I don't understand you :( Check this page and try again please

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.

3 participants