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

Refactor rustic_core API & add more documentation to rustic_core #787

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

aawsome
Copy link
Member

@aawsome aawsome commented Jul 27, 2023

closes rustic-rs/rustic_core#8

TODO

  • write concise entry comment in rustic_core/Readme.md (factored out into docs: Update Readmes #820)
  • write concise entry comment in lib.rs
  • recheck imports and library API
    • regenerate public API data (see Justfile)
  • document errors in fallible functions and methods
  • document # Type Parameters for Repository and others
  • fully document methods and functions in public API

Moved to umbrella issue

  • remove unwraps out of public API (e.g. /commands/copy::copy()) and in general out of the core library
  • Replace logging with error handling, there shouldn't be error! anywhere in the API of the library, they should be actual errors

@aawsome aawsome added A-docs Area: Improvements or additions to documentation A-commands Area: Related to commands in `rustic` A-core Area: Generally related to `rustic_core` A-architecture Area: Related to `rustic`s architecture A-examples Area: Related to examples labels Jul 27, 2023
@aawsome aawsome force-pushed the refactor-api branch 2 times, most recently from 12783d0 to 91b8c8d Compare July 30, 2023 20:33
@simonsan
Copy link
Contributor

Can you tag me, when I should review? Force-pushing makes it relatively hard to follow changes between each read.

@aawsome aawsome force-pushed the refactor-api branch 3 times, most recently from 1578dd3 to 19e5f01 Compare August 1, 2023 18:13
@aawsome
Copy link
Member Author

aawsome commented Aug 1, 2023

@simonsan It is far from perfect, but I think it is a good start which would enable us to publish a first version of the rustic_core crate.
I am very looking forward to feedback!

@simonsan
Copy link
Contributor

simonsan commented Aug 3, 2023

@simonsan It is far from perfect, but I think it is a good start which would enable us to publish a first version of the rustic_core crate. I am very looking forward to feedback!

Will take some time today/the next days to go through it!

Copy link
Contributor

@simonsan simonsan left a comment

Choose a reason for hiding this comment

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

First part (please don't force push from now on, otherwise my progress will be lost for the rest of the files).

crates/rustic_core/examples/backup.rs Outdated Show resolved Hide resolved
crates/rustic_core/examples/config.rs Outdated Show resolved Hide resolved
crates/rustic_core/examples/key.rs Show resolved Hide resolved
crates/rustic_core/examples/merge.rs Outdated Show resolved Hide resolved
crates/rustic_core/examples/ls.rs Show resolved Hide resolved
crates/rustic_core/src/backend/node.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/blob.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/blob.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/commands/config.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/commands/forget.rs Show resolved Hide resolved
@aawsome aawsome added the C-refactor Category: Refactoring of already existing code label Aug 6, 2023
@aawsome
Copy link
Member Author

aawsome commented Aug 7, 2023

I just force-pushed the last commit to correct compile problems from the suggestions (last_modified_node wasn't used at all places)

@aawsome aawsome mentioned this pull request Aug 8, 2023
Copy link
Contributor

@simonsan simonsan left a comment

Choose a reason for hiding this comment

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

Second review batch, came until /src/repofile/indexfile.rs 👍🏽

Maybe you can go through the imports again, because in most files these are wrong (see some comments).

Note: Still three open issues from last review.

crates/rustic_core/src/backend/local.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/backend/local.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/backend/local.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/backend/local.rs Show resolved Hide resolved
crates/rustic_core/src/backend/local.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/configfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/configfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/configfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/configfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/configfile.rs Outdated Show resolved Hide resolved
@simonsan simonsan added the P-high Priority: High, to be fixed at a higher priority label Aug 8, 2023
@simonsan
Copy link
Contributor

simonsan commented Aug 8, 2023

Gave this a P-high as I think we should merge and focus on this big PR before merging others. So we don't open more than one construction site at once.

crates/rustic_core/src/repofile/indexfile.rs Show resolved Hide resolved
crates/rustic_core/src/repofile/indexfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/packfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/packfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repofile/packfile.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repository.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repository.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repository.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repository.rs Outdated Show resolved Hide resolved
crates/rustic_core/src/repository.rs Outdated Show resolved Hide resolved
@simonsan
Copy link
Contributor

Fixed all or most of the issue I could spot.

Some traits need further documentation:

pub trait IndexedTree

pub trait IndexedIds: IndexedTree {}

pub trait IndexedFull: IndexedIds {}

Also, a good search string: // TODO: Add documentation

@simonsan simonsan requested a review from a team August 13, 2023 02:42
@simonsan simonsan changed the title Refactor & document rustic_core API Refactor rustic_core API & add more documentation to rustic_core Aug 13, 2023
@simonsan simonsan force-pushed the refactor-api branch 2 times, most recently from 5c9d85c to 7ede0a9 Compare August 17, 2023 00:27
@simonsan
Copy link
Contributor

simonsan commented Aug 17, 2023

I also added the following, I can factor that out into (a) separate PR(s) if you want to:

Factored out:

PR #820:

  • testing, releasing, and development guide
  • text changes to rustic and rustic_core readmes
  • graphical and layout changes incl. the addition of a logo
  • changes to CONTRIBUTING.md

PR #821:

  • fail fast for the matrix CI

PR #822:

  • Update the manifests to prepare for rustic_core release

Co-authored-by: Alexander Weiss <alex@weissfam.de>
@aawsome
Copy link
Member Author

aawsome commented Sep 5, 2023

This PR unfortunately got a bit messy. I started several times to work on it but always ran out of time (or motivation, to be honest) to further continue.. This is something we have to improve - make smaller PRs which stay reviewable!

So, today another approach.. I removed quite some commits from you @simonsan - the ones which you have factored out and I hope I didn't accidentially remove something important.

@aawsome
Copy link
Member Author

aawsome commented Sep 5, 2023

I also fixed some non-running tests and squashed the commits...

@aawsome
Copy link
Member Author

aawsome commented Sep 5, 2023

I won't say that this PR is finishing all attempts to define&document the rustic_core API, but it improves it a lot and is a big PR which always stands in the way if we don't merge it.

As it looks very good from my side, I'll merge it to make the way open for (hopefully much smaller/ more local) further PRs which even more improve it.

@aawsome aawsome merged commit 97d0774 into main Sep 5, 2023
20 checks passed
@aawsome aawsome deleted the refactor-api branch September 5, 2023 20:35
@simonsan
Copy link
Contributor

simonsan commented Sep 5, 2023

This is something we have to improve - make smaller PRs which stay reviewable!

I agree, though for lacking documentation it's pretty reasonable I think. In the end, it was technical debt that we had to work on which (IMHO, as expected) turned out to be quite a huge PR. We should keep that in mind for future PRs regarding the scope crates/rustic_core/* to always add documentation so we are not increasing that debt again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-architecture Area: Related to `rustic`s architecture A-commands Area: Related to commands in `rustic` A-core Area: Generally related to `rustic_core` A-docs Area: Improvements or additions to documentation A-examples Area: Related to examples C-refactor Category: Refactoring of already existing code P-high Priority: High, to be fixed at a higher priority
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Document public rustic_core API
2 participants