Skip to content

api(rust): Add api endpoint get_status_update#4468

Merged
Septias merged 9 commits intomasterfrom
sk/new_apis
Jun 15, 2023
Merged

api(rust): Add api endpoint get_status_update#4468
Septias merged 9 commits intomasterfrom
sk/new_apis

Conversation

@Septias
Copy link
Copy Markdown
Collaborator

@Septias Septias commented Jun 9, 2023

Some small core-changes I made while working on the appstore-bot.
To be able to close webxdc/store#35 we need to merge this changes into core or change the bot to not use the new apis.

@Septias Septias changed the title Compatibility changes for appstore-bot feat: Compatibility changes for appstore-bot Jun 9, 2023
@Septias Septias requested review from Hocuri, link2xt and r10s June 9, 2023 14:00
@Hocuri
Copy link
Copy Markdown
Collaborator

Hocuri commented Jun 12, 2023

I can't really comment on the API itself because I'm not involved with Webxdc a lot, so @link2xt will have to do this.

@Septias Septias changed the title feat: Compatibility changes for appstore-bot api(rust): Add api to send WebxdcUpdateStruct Jun 13, 2023
@Septias Septias changed the title api(rust): Add api to send WebxdcUpdateStruct api(rust): Add api endpoint get_status_update Jun 13, 2023
Copy link
Copy Markdown
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

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

Looks good, but many unrelated changes piggybacked in the PR without any explanation why they are needed.

#[derive(Debug, Deserialize, Default)]
#[non_exhaustive]
struct WebxdcManifest {
pub struct WebxdcManifest {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Making this public seems to be a completely unrelated change, not needed for get_status_update at all, but will not be reflected in the changelog.


/// Update items as sent on the wire and as stored in the database.
#[derive(Debug, Serialize, Deserialize)]
#[derive(Debug, Serialize, Deserialize, Default)]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Default derivation is also an unrelated refactoring

src/webxdc.rs Outdated
Ok(status_update_serial)
}

/// Return the update_item with `status_update_serial` from the webxdc with message id `msg_id`.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Double space in the beginning.

Should start with "Returns", not "Return".

What is update_item? It is some database column, an implementation detail, does not explain what you will get.
Ideally should use the terminology from https://docs.webxdc.org/spec.html, there it is called just an "update".

request_internet_access: None,
}
};
let mut manifest = get_blob(&mut archive, "manifest.toml")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What happened here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

refactoring based on the default derivement

@Septias
Copy link
Copy Markdown
Collaborator Author

Septias commented Jun 14, 2023

Was originally just a toy branch for me, I can create a new Branch an Pr if you prefer.

@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Jun 14, 2023

Was originally just a toy branch for me, I can create a new Branch an Pr if you prefer.

Does not matter that much, also ok to merge as a single commit after fixing the doc comment. I'll try to make a release soon anyway and hope not to forget to document the changes manually when making a release.

@link2xt
Copy link
Copy Markdown
Collaborator

link2xt commented Jun 14, 2023

cargo-deny check is fixed on master, CI will be green once this PR is rebased.

@Septias Septias merged commit 768f817 into master Jun 15, 2023
@Septias Septias deleted the sk/new_apis branch June 15, 2023 13:35
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

Successfully merging this pull request may close these issues.

Depend on the released version of the core

3 participants