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

feat: gateway upgrade #87

Merged
merged 6 commits into from
Jun 1, 2022
Merged

feat: gateway upgrade #87

merged 6 commits into from
Jun 1, 2022

Conversation

Arqu
Copy link
Collaborator

@Arqu Arqu commented May 30, 2022

@Arqu Arqu self-assigned this May 30, 2022
@Arqu Arqu force-pushed the arqu/gateway_stuff branch from 4eb8c1f to c7d2bc0 Compare May 30, 2022 12:04
@Arqu Arqu force-pushed the arqu/gateway_stuff branch from 6d8ea20 to 3854216 Compare May 31, 2022 12:35
@Arqu Arqu marked this pull request as ready for review May 31, 2022 12:35
@Arqu Arqu requested a review from dignifiedquire May 31, 2022 12:36
@Arqu Arqu force-pushed the arqu/gateway_stuff branch from 975e4fd to 89a5f4d Compare May 31, 2022 20:02
state,
));
}
let mut uri_path = u.path().to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for to_string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Required for the write! 4 lines lower.

Copy link
Contributor

Choose a reason for hiding this comment

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

write takes &str happily

query_file_name,
content_path: cpath.to_string(),
content_path: full_content_path.to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe store the Path directly instead of the stringified version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of the smaller manipulations require details from the explicit path that was passed in. Some of those details get lost on the resolver::Path like whether the path had a trailing / at the end or not. Opening an issue as most of them are able to utilize it properly though.

Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

Some smaller stuff. Biggest issue for me is missing tests for a lot of logic changes/additions, fine to punt for another PR, but should at least have an issue to start tracking

@Arqu
Copy link
Collaborator Author

Arqu commented May 31, 2022

Some smaller stuff. Biggest issue for me is missing tests for a lot of logic changes/additions, fine to punt for another PR, but should at least have an issue to start tracking

My next quest is integration testing. Opening an issue to stay on top of it.

@Arqu Arqu requested a review from dignifiedquire May 31, 2022 21:11
@Arqu Arqu merged commit 4732a76 into main Jun 1, 2022
@Arqu Arqu deleted the arqu/gateway_stuff branch June 1, 2022 07:11
dignifiedquire added a commit that referenced this pull request Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

clean up gateway
2 participants