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

Fix graph node instance #30

Merged
merged 4 commits into from
Aug 17, 2023
Merged

Fix graph node instance #30

merged 4 commits into from
Aug 17, 2023

Conversation

aasseman
Copy link
Contributor

Details in commit messages

@aasseman
fix: subgraphs_base_url trailing /
f80b7af 
Signed-off-by: Alexis Asseman <[email protected]>
@aasseman
fix: graph-node reqwest response json escaping
1890501 
Reqwest reponse body gets escaped when decoding using `.text()`. Using
`.json()` (without json schema hints) still delivers a string, but it
is not escaped, thus easier to deserialize from json.

Signed-off-by: Alexis Asseman <[email protected]>
@aasseman aasseman added the bug label Aug 15, 2023
@aasseman aasseman requested a review from hopeyen August 15, 2023 21:26
@aasseman aasseman self-assigned this Aug 15, 2023
@coveralls
Copy link

coveralls commented Aug 15, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5884343056

  • 63 of 106 (59.43%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+8.2%) to 13.853%
Changes Missing Coverage Covered Lines Changed/Added Lines %
service/src/graph_node.rs 63 106 59.43%
Totals Coverage Status
Change from base Build 5871688573: 8.2%
Covered Lines: 206
Relevant Lines: 1487
💛 - Coveralls

@hopeyen
Copy link
Collaborator

hopeyen commented Aug 16, 2023

The code looks good, would you be able to post some examples of what the expected requests look like?
I'm asking because the previous format doesn't seem to process anymore and I started to get "Empty reply from server"

@aasseman
Copy link
Contributor Author

🔴 as expected!

@aasseman
Copy link
Contributor Author

And now 🟢 !
Though it looks that Semantic PR is not set to accept revert commits

hopeyen
hopeyen approved these changes Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@hopeyen hopeyen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm!
I will request an update to our org semantics settings for revert commits, you could also rewrite the commit message, or drop the commit (don't know if it is more conventional to use Merge and Revert commits, I usually prefer rebase and squash commits for a linear and concise git history

@aasseman
Copy link
Contributor Author

lgtm! I will request an update to our org semantics settings for revert commits, you could also rewrite the commit message, or drop the commit (don't know if it is more conventional to use Merge and Revert commits, I usually prefer rebase and squash commits for a linear and concise git history

Yup, we can definitely squash this one

@aasseman aasseman merged commit 4bfd72c into main Aug 17, 2023
4 checks passed
@aasseman aasseman deleted the fix_graph_node_instance branch August 17, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hopeyen hopeyen hopeyen approved these changes

Assignees

@aasseman aasseman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aasseman @coveralls @hopeyen