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

impl rust Collaboration and Remote API #5773

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

rbran
Copy link
Contributor

@rbran rbran commented Jul 19, 2024

Unimplemented and other notes:

  • BNRemoteFileDownload - Returns the c++ std::vector from the core API, but don't provide any way to free it.
  • BNCollaborationSnapshotDownloadSnapshotFile - Returns the c++ std::vector from the core API, but don't provide any way to free it.
  • BNRemoteRequest - Parameters are complex c++ types, and no direct way to manage it.
  • BNAnalysisMergeConflictGetPathItem - Implementation with downcast to u64 is unclear.
  • Remote::connect - Not fully implemented because it depends on enterprise and SecretsProvider.
  • Remote - Methods in python preemptively calls pull_files/pull_folders/pull_user_permissions/open, but C++ don't. Python was followed.
  • RemoteProject::upload_new_file - Was ported from remotebrowser.cpp, this file is not public, so I decided not to implement it.

Copy link
Member

@emesare emesare left a comment

Choose a reason for hiding this comment

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

I got through 18 before I realized how large this PR is.

For actual failures like merge_database a Result type would be nice, however in the absence of an actual error type it is a code smell. If you added just a simple MergeFailed type and attached it I think that would be better. This is pretty subjective so I'm sure others will have differing opinions.

rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/databasesync.rs Show resolved Hide resolved
rust/src/collaboration/changeset.rs Show resolved Hide resolved
rust/src/collaboration/changeset.rs Show resolved Hide resolved
@rbran
Copy link
Contributor Author

rbran commented Jul 19, 2024

It's probably better to leave Result<T, ()> for now, because it's very likely that an Error type will be implemented and it will be replaced byResult<T, BnError> .

Although it's not ideal, it's the current API behavior:

pub fn push(&self, value: &Metadata) -> Result<(), ()> {

@rbran rbran marked this pull request as ready for review July 22, 2024 12:30
@CouleeApps CouleeApps added the Component: Rust API Issue needs changes to the Rust API label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Rust API Issue needs changes to the Rust API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants