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

Improve code for toml file handling and remove code duplication from new.rs file #152

Closed
Bas-Man opened this issue Jan 24, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@Bas-Man
Copy link
Contributor

Bas-Man commented Jan 24, 2023

I have been looking at code regarding issue #130 and which was referenced by pull request number #142.
I have some comments and ideas on how to improve this.

Issues

  • File new.rs contains a lot of information regarding how Site and Issue struct data is to be written to their toml files. I think this can be removed by making struct Site{} and struct Issue{} directly responsible for writing their related data to file using the the toml crate.
  • When creating a new Issue there currently does not appear to be any protection to prevent a user from intentionally overwrite an exiting Issue.

Ideas

@Folyd Folyd added the enhancement New feature or request label Jan 27, 2023
@Bas-Man
Copy link
Contributor Author

Bas-Man commented Jan 27, 2023

These drafts are ready for a quick review and comments.
I have done it this way to avoid breaking any existing code. And to also make it easier to avoid merge conflicts.
Once this pull requests are complete and merged, the code in new.rs can be updated to use them. There may need to be some additional fixes during that process.
@Folyd

@Bas-Man
Copy link
Contributor Author

Bas-Man commented Feb 6, 2023

I will submit a new pull request to replace the ones I have just closed.

@Bas-Man
Copy link
Contributor Author

Bas-Man commented Feb 8, 2023

I have issued a new pr #162 which will close this issue #152

This should make it much easier to add new features. And also make it much easier to add the zine new --article support.

@Bas-Man Bas-Man closed this as not planned Won't fix, can't repro, duplicate, stale Mar 10, 2023
@Bas-Man
Copy link
Contributor Author

Bas-Man commented Mar 16, 2023

@Folyd

Can you tell me what issues you have with my three PRs. They are all related. Each pr builds on the previous to create the desired "--article" command line option.

I can make one more attempt to get to something you can work with. But they all need to be implemented for the "--article" option to work.

@Folyd
Copy link
Member

Folyd commented Mar 19, 2023

Hi, @Bas-Man. I apologize for the delay in my response. Thanks for working on this. I hope you can submit a single PR with as less changes as possible to support the --article. What do you think?

@Bas-Man
Copy link
Contributor Author

Bas-Man commented Mar 19, 2023

Nearly all the changes are required. Each change builds on previous to make things clean and maintainable moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants