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

Change schema to allow alternative configurations #20

Merged
merged 10 commits into from
Sep 4, 2023
Merged

Change schema to allow alternative configurations #20

merged 10 commits into from
Sep 4, 2023

Conversation

ipetkov
Copy link
Contributor

@ipetkov ipetkov commented Sep 3, 2023

Refs #2
Refs #13 (comment)

@ipetkov ipetkov marked this pull request as ready for review September 3, 2023 23:14
README.md Outdated Show resolved Hide resolved
@srid srid added this to the 0.2 milestone Sep 3, 2023
Copy link
Owner

@srid srid left a comment

Choose a reason for hiding this comment

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

Instead of introducing a new indirection type (FlakeRefKind), I'd just modify the PulRequestRef type to take an additional struct field called anchor. Then, PullRequest::flake_url can construct a flake url that uses the anchor as attribute.

Incidentally, this would simplify the diff even further (which is something you want to aim for in first-pass attempts).

@srid
Copy link
Owner

srid commented Sep 4, 2023

pub fn flake_url(&self) -> String 

This function can be refactored to return a FlakeUrl type (newtype over String) which then impl's a method to get the attribute.

@ipetkov
Copy link
Contributor Author

ipetkov commented Sep 4, 2023

Instead of introducing a new indirection type (FlakeRefKind), I'd just modify the PulRequestRef type to take an additional struct field called anchor. Then, PullRequest::flake_url can construct a flake url that uses the anchor as attribute.

I've applied this feedback, let me know if you have anything else in mind!

This function can be refactored to return a FlakeUrl type (newtype over String) which then impl's a method to get the attribute.

I did not apply this feedback because it resulted in having to wrap the url String in multiple places only to unwrap it later and it did not seem like the boilerplate was adding much. But if you want this refactor done I can add it in

@srid
Copy link
Owner

srid commented Sep 4, 2023

@ipetkov This is fine. I just realized that there is more to it than FlakeUrl, so I'll take over this PR and merge it to mainline.

@srid srid self-assigned this Sep 4, 2023
@srid srid merged commit 0b85003 into srid:master Sep 4, 2023
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