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.

Sign up for GitHub

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

Jump to bottom

Document processing v2 #669

Merged
merged 8 commits into from
Nov 27, 2023
Merged

Document processing v2 #669

merged 8 commits into from
Nov 27, 2023

Conversation

Siegrift
Copy link
Contributor

Relates to api3dao/commons#27

Rationale

Read api3dao/commons#32

@Siegrift Siegrift requested review from wkande and dcroote November 18, 2023 17:38
@Siegrift Siegrift self-assigned this Nov 18, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 18, 2023 •
edited
Loading

Visit the preview URL for this PR (updated for commit 288d014):

https://vitepress-docs--pr669-processing-v2-d3oh59pa.web.app

(expires Mon, 04 Dec 2023 08:10:42 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: e4c5db1acb62b36273dc03718b86834917dea599

@Siegrift Siegrift mentioned this pull request Nov 18, 2023
Closed
5 tasks
bbenligiray
bbenligiray reviewed Nov 20, 2023
View reviewed changes
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Show resolved Hide resolved
@bbenligiray
Copy link
Member

One thing to note is that it's possible to specify an endpoint with no API call, so apiCallParameters, apiCallResponse, etc. are not suitable. A more generic naming would be more compatible with other kinds of operations (other than API calls) or more modular stuff like chained API calls, etc. that may be implemented in the future.

dcroote
dcroote reviewed Nov 21, 2023
View reviewed changes
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/specification.md Outdated Show resolved Hide resolved
@api3dao api3dao deleted a comment from dcroote Nov 21, 2023
@bbenligiray
Copy link
Member

I left a few small but imo somewhat important comments. If you respond and need me to respond, feel free to poke me with a DM.

@api3dao api3dao deleted a comment from dcroote Nov 23, 2023
@Siegrift Siegrift requested a review from dcroote November 25, 2023 07:33
@Siegrift
Copy link
Contributor Author

I think the discussion is now done. @dcroote @wkande can you take a final look and approve?

dcroote
dcroote approved these changes Nov 27, 2023
View reviewed changes
Copy link
Collaborator

@dcroote dcroote left a comment

Choose a reason for hiding this comment

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

Only minor / nits, otherwise LGTM 👍

docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
docs/reference/ois/next/processing.md Outdated Show resolved Hide resolved
@Siegrift Siegrift merged commit daeff84 into main Nov 27, 2023
2 checks passed
@Siegrift Siegrift deleted the processing-v2 branch November 27, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbenligiray bbenligiray bbenligiray left review comments

@dcroote dcroote dcroote approved these changes

@wkande wkande Awaiting requested review from wkande

Assignees

@Siegrift Siegrift

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Siegrift @bbenligiray @dcroote