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

Make Hex API and CDN URLs configurable #36

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

diemogebhardt
Copy link

@diemogebhardt diemogebhardt commented Jan 25, 2025

This PR is regarding issue gleam-lang/gleam#2817 and allows for configuration of Hex URLs.

Breaking Change

It changes the primary way to create a Config from Config::new() to Config::default().

// Use default API and repository URL
let config = Config::default()

Configuration

It introduces two builder methods for customizing the Hex URLs independently:

  • with_custom_api allows setting a custom API URL
  • with_custom_repository allows setting a custom repository URL

These methods can be used individually or chained together when both URLs need to be customized:

// Use custom API URL and default repository URL
let config = Config::default()
    .with_custom_api("https://example.com/api/".parse().unwrap());

// Use default API URL and custom repository URL
let config = Config::default()
    .with_custom_repository("https://repo.example.com/".parse().unwrap());

// Use custom URLs for both
let config = Config::default()
    .with_custom_api("https://example.com/api/".parse().unwrap())
    .with_custom_repository("https://repo.example.com/".parse().unwrap());

@@ -27,7 +27,7 @@ regex = "1.3"
# Byte collections
bytes = "1"
# Protobuf runtime
protobuf = "3.4"
protobuf = "=3.4"
Copy link
Author

Choose a reason for hiding this comment

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

Version 3.7.1 of the protobuf crate has been released and failed the build.

But I didn't want to overload this PR so I am going to bump the protobuf version and regenerate the files in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

See: PR #37

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you, but it's not appropriate for a library to be configured using environment variables or to print to the console or to halt the program, those are impure actions that applications would need to opt-in to and perform themselves. Instead the configuration would need to be passed in, and then the applications can decide what would be an appropriate manner to decide what to set those values to.

Could you convert that over please? Then we can read environment variables in the Gleam build tool 🙏

This change aligns better with Rust conventions by using Default to provide
standard Hex URLs, making the common case easier and more idiomatic. Since
Config has well-defined defaults (hex.pm URLs), implementing Default is more
appropriate than using a new() constructor.
@diemogebhardt diemogebhardt changed the title Make Hex API and CDN URLs configurable using Environment Variables Make Hex API and CDN URLs configurable Jan 30, 2025
@diemogebhardt
Copy link
Author

diemogebhardt commented Jan 30, 2025

Could you convert that over please? Then we can read environment variables in the Gleam build tool 🙏

Yes, that makes way more sense!

Configuration and Breaking Changes

I hope it's ok that I introduced a breaking change to use Config::default() over Config::new()?

// Use default API URL and repository URL
let config = Config::default()

// Use custom API URL and default repository URL
let config = Config::default()
    .with_custom_api("https://example.com/api/".parse().unwrap());

// Use default API URL and custom repository URL
let config = Config::default()
    .with_custom_repository("https://repo.example.com/".parse().unwrap());

// Use custom URLs for both
let config = Config::default()
    .with_custom_api("https://example.com/api/".parse().unwrap())
    .with_custom_repository("https://repo.example.com/".parse().unwrap());

The default is to use the standard Hex URLs without having to set them explicitly at all, so using Config::default() feels more natural to me. I can change it back to Config::new() in case you prefer that though.

Naming and Consistency

You proposed HEX_API_URL and HEX_CDN_URL as environment variable names which is clear and understandable.

In the context of this library we use api_base and repository_base internally. Shall we rename everything repository to cdn (for example: cdn_base and with_custom_cdn) in order to use one consistent naming scheme throughout?

@diemogebhardt diemogebhardt force-pushed the configure-hex-api-and-cdn-urls-using-env-vars branch 2 times, most recently from 133b4e8 to b06dcab Compare February 2, 2025 12:11
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.

2 participants