Skip to content
This repository has been archived by the owner on May 2, 2024. It is now read-only.

feat: persistance system #61

Open
wants to merge 19 commits into
base: oxidation
Choose a base branch
from
Open

Conversation

saenai255
Copy link

@saenai255 saenai255 commented Sep 1, 2022

Closes #28

@saenai255 saenai255 changed the base branch from develop to oxidation September 1, 2022 17:00
@saenai255 saenai255 self-assigned this Sep 1, 2022
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #61 (8eb4a20) into oxidation (88764fe) will decrease coverage by 42.98%.
The diff coverage is 54.51%.

@@              Coverage Diff               @@
##           oxidation      #61       +/-   ##
==============================================
- Coverage     100.00%   57.01%   -42.99%     
==============================================
  Files              1        7        +6     
  Lines             22      328      +306     
==============================================
+ Hits              22      187      +165     
- Misses             0      141      +141     
Impacted Files Coverage Δ
src/config.rs 100.00% <ø> (ø)
src/model/pacbuild.rs 4.44% <4.44%> (ø)
src/store/filters.rs 40.00% <40.00%> (ø)
src/store/base.rs 57.30% <57.30%> (ø)
src/store/errors.rs 75.00% <75.00%> (ø)
src/store/query_builder.rs 82.14% <82.14%> (ø)
src/model/repository.rs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@wizard-28 wizard-28 linked an issue Sep 2, 2022 that may be closed by this pull request
@wizard-28 wizard-28 added the enhancement New feature or request label Sep 5, 2022
@saenai255 saenai255 marked this pull request as ready for review September 5, 2022 16:48
Copy link
Member

@wizard-28 wizard-28 left a comment

Choose a reason for hiding this comment

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

Well, that's my quick review, this PR will take a few more days to fully review.

General impressions:

  • Add more docs, the whole fs module's missing docs.
  • Look into error handling libraries
  • 13 of the 16 tests fail, check the Code Coverage CI.

src/model/pacbuild.rs Outdated Show resolved Hide resolved
src/model/pacbuild.rs Show resolved Hide resolved
src/model/pacbuild.rs Outdated Show resolved Hide resolved
src/model/mod.rs Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/errors.rs Outdated Show resolved Hide resolved
src/store/filters.rs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/fs.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
saenai255 and others added 11 commits October 4, 2022 19:11
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Copy link
Member

@wizard-28 wizard-28 left a comment

Choose a reason for hiding this comment

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

Add code examples to the public methods detailing how to use them.

Also, add a full-fledged example(s) on how to use the database module, in examples/ folder on the root of the project.

So people can run cargo run --example <database_example> to run the example.

src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
src/store/base.rs Outdated Show resolved Hide resolved
Co-authored-by: Sourajyoti Basak <wiz28@protonmail.com>
Copy link
Member

@wizard-28 wizard-28 left a comment

Choose a reason for hiding this comment

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

Can you rename the PacBuild fields to resemble their PACBUILD counterparts wherever possible? Example: package_names -> pkgname, homepage -> url, licenses -> license

Comment on lines +43 to +47
/// Repology filter.
///
/// # Example
/// **TBA**
pub repology: String,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a HashMap of the filters?

///
/// # Example
/// `Paul Cosma <paul.cosma97@gmail.com>`
pub maintainers: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub maintainers: Vec<String>,
pub maintainers: Vec<String>,

Shoud be optional

pub description: String,

/// Official homepage [URL].
pub homepage: URL,
Copy link
Member

Choose a reason for hiding this comment

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

Shoud be optional

pub homepage: URL,

/// Latest version fetched from Repology.
pub repology_version: Version,
Copy link
Member

Choose a reason for hiding this comment

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

Why should we store the repology version?

Comment on lines +57 to +59
/// An array of packages that must be installed for the software to build
/// and run.
pub dependencies: Vec<VersionConstrainedPackageId>,
Copy link
Member

Choose a reason for hiding this comment

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

Shoud be optional

Comment on lines +86 to +88
/// The group the package belongs in. For instance, when installing
/// `plasma`, it installs all packages belonging in that group.
pub groups: Vec<GroupId>,
Copy link
Member

Choose a reason for hiding this comment

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

Shoud be optional

Comment on lines +90 to +92
/// Optional dependencies. Each Key:Pair is meant to describe the package
/// identifier and the reason for installing.
pub optional_dependencies: HashMap<VersionConstrainedPackageId, String>,
Copy link
Member

Choose a reason for hiding this comment

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

Shoud be optional

Comment on lines +94 to +95
/// An array of packages that are only required to build the software.
pub make_dependencies: Vec<VersionConstrainedPackageId>,
Copy link
Member

Choose a reason for hiding this comment

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

Shoud be optional

Comment on lines +97 to +98
/// The license under which the software is distributed.
pub licenses: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Should be optional

Comment on lines +100 to +101
/// File required to build the package.
pub url: URL,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to store the url in the database?

@Elsie19 Elsie19 changed the title Feat/persistance system feat: persistance system Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a flexible cache system
2 participants