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

Refactor ResourceRef expansion #4450

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

dantb
Copy link
Contributor

@dantb dantb commented Oct 31, 2023

Very small refactor for deduplication. However it's a good example of something done a lot in the codebase - using IO when a weaker data structure would suffice. I find it easier to reason about when this is pushed to call sites since it's clear at a glance that nothing external is involved.

For example here you might think that creating a RootSearch or an OrgSearch does I/O or something, when it's just some parsing which can fail. I think if this approach is adopted more generally, you'll find many more opportunities to refactor / deduplicate and it'll hopefully be more readable. Also testing should become easier, though it probably won't impact integration-style tests as much.

Not sure how much of this is personal preference, so I'm all ears 😄

Copy link
Contributor

@shinyhappydan shinyhappydan left a comment

Choose a reason for hiding this comment

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

I don't really have a strong opinion on this but it feels like Try is a better fit here?

@dantb
Copy link
Contributor Author

dantb commented Nov 1, 2023

I don't really have a strong opinion on this but it feels like Try is a better fit here?

Hmm I'd agree if we were throwing, but we're only using Throwable as the error channel in a "pure" way here - there's no messing with control flow like with exceptions. Try is ideal for stuff like a FFI to Java code for instance

Copy link
Contributor

@olivergrabinski olivergrabinski left a comment

Choose a reason for hiding this comment

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

I agree that having Either instead of IO in such cases is probably easier to reason about. I don't particularly like having Throwable as the error type; we have more specific types and I think we could use that?

@dantb dantb merged commit 2462ec7 into BlueBrain:master Nov 2, 2023
5 checks passed
@dantb dantb deleted the refactor-resource-ref-expansion branch November 2, 2023 10:43
@dantb
Copy link
Contributor Author

dantb commented Nov 2, 2023

I agree that having Either instead of IO in such cases is probably easier to reason about. I don't particularly like having Throwable as the error type; we have more specific types and I think we could use that?

Forgot to do this here but you're right it could be Rejection. Changed in #4455

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.

3 participants