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

Template Updates #8

Merged
merged 4 commits into from
Apr 21, 2020
Merged

Template Updates #8

merged 4 commits into from
Apr 21, 2020

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Apr 14, 2020

Template Changes

Here are some small changes to our template program. These are intended to address a few corners I've seen new users running into, namely: setting the correct backend name, and how users should handle errors in our application.

Overview

The most important changes are...

  • Bump fastly to version 0.3.0
  • Use the new #[fastly::main] procedural macro. (See: https://docs.rs/fastly/0.3.0/fastly/attr.main.html)
  • This moves our "backend_name" literals up to the top of the file, into documented const values. This helps highlight the parts of our template that a user will always need to change, so people do not forget and run into cryptic hostcall error messages.
  • This also adds a small comment to the main documentation explaining that returning an error will result in a 500 Internal Server Error response being sent back to the client. I think this is a small conceptual detail that isn't explained clearly currently.

Small Changes

A few other small changes made in the process of this PR:

  • Adds documentation comments to elements.
  • Moves the VALID_METHODS array into our handle_request function. This was done to try and emphasize the BACKEND_NAME consts, which must be changed for someone's service to work.

src/main.rs Outdated Show resolved Hide resolved
@cratelyn cratelyn requested review from phamann and triblondon April 14, 2020 22:25
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@triblondon triblondon left a comment

Choose a reason for hiding this comment

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

Left a few comments. I do think the new comment format makes it less clear. No real opinion on the order of the functions. Moving backend names into constants is a good idea.

Katelyn Martin added 3 commits April 20, 2020 14:41
This makes a few minor stylistic changes to our `main.rs` template:

* use builder to generate internal error response
* describe error case in doc comment
* move `main` to top of program
* move valid methods array into `handle_request`
* move backend constants to top, add docs
This PR removes the links in the documentation comments of `main`
and `handle_request` in our template.

While Rust documentation comments often include markdown links, this
program template actually isn't a fantastic place to use this
functionality.

Most often, these comments will be viewed directly in the source code.

Another important point against these is that neither of these functions
are exported with `pub`. Consequentially, they won't show up in `cargo
doc --open`. At best, they would appear in auto-completion dialogues
in editors like VSCode, but this benefit isn't worth the impact to
readability.
@cratelyn cratelyn force-pushed the ktm/service-error-response-builder branch from 166ebef to 3be792c Compare April 20, 2020 18:46
Cargo.toml Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@cratelyn cratelyn requested a review from triblondon April 20, 2020 19:16
Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@cratelyn cratelyn requested a review from acfoltzer April 21, 2020 21:50
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

Looks great!

@cratelyn cratelyn merged commit a1aefd7 into master Apr 21, 2020
@cratelyn cratelyn deleted the ktm/service-error-response-builder branch April 21, 2020 22:50
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.

5 participants