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

Contributing to this Repo: remove panic and unwrap #5

Closed
Bas-Man opened this issue Feb 27, 2023 · 6 comments
Closed

Contributing to this Repo: remove panic and unwrap #5

Bas-Man opened this issue Feb 27, 2023 · 6 comments

Comments

@Bas-Man
Copy link
Contributor

Bas-Man commented Feb 27, 2023

Hi,
I am interested in contributing to this repo. I was thinking of writing my own. But this seems to be a great place to start and contribute.

Some of the issues that I see in the code that I think I can help with.

  • Code has a panic hard coded. I would like to remove this and make the function return a result
  • There are a number of unwraps that might fail on a malformed xml file.
  • Have the program handleparsing a directory as a Path in a nice way.

I have already raised two PRs to address some very simple things.

  • Prevent the tests for failing with test_parse_dir by adding the #[should_panic] See pr Add test to expect panic #3
  • Made parse_reader private as this does not need to be public in the current code.
@bbustin
Copy link
Owner

bbustin commented Feb 27, 2023

Hi @Bas-Man. I agree, there are a lot of areas this could be improved. I really hacked it together quickly while initially learning Rust. Your help is much appreciated. Removing the panics and unwraps would be great.

I incorrectly closed the pull request about making parse_reader private. You are correct. It should not be public. I mis-read it as parse and closed the pull request. I will try to re-open and merge it. Sorry about that...

The second pull request is also a good one. Ideally parse_dir would not try to parse files that are not .xml, .gz, or .zip. But for now making this change is a good idea.

@Bas-Man
Copy link
Contributor Author

Bas-Man commented Feb 27, 2023

Yes. Ideally parse_dir would only parse valid files. The [should_panic] is a temp fix to get all tests passing including the panic.

I am already thinking about how to resolve the parse_dir so that it does not actually panic.

But moving step by step to avoid conflicts.

I also would like to make most of the struct data private with accessors. But that might be over engineering.

@Bas-Man
Copy link
Contributor Author

Bas-Man commented Feb 28, 2023

@bbustin could you point to a good source of demarc samples.

@bbustin
Copy link
Owner

bbustin commented Mar 1, 2023

I do not know a good source of samples, unfortunately. I was just using ones I was receiving on my mail server from Google, Yahoo, and other places.

@Bas-Man
Copy link
Contributor Author

Bas-Man commented Mar 2, 2023

No worries. I will work with what we have.

I would like to remove the guard in parse_dir()

The path.is_dir() {}
This appears to be preventing any IO errors from being returned. This is because it evaluates before any IO operations. At least from what I have tried. When it is removed. IO errors are returned for unreadable files and directories.

@bbustin
Copy link
Owner

bbustin commented Mar 8, 2023

👍

@bbustin bbustin closed this as completed Mar 8, 2023
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

No branches or pull requests

2 participants