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

Use read_file macro in Crystal::Config.version #7081

Merged
merged 1 commit into from
Feb 7, 2019

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Nov 13, 2018

Followup to #6967. If accepted, needs to be merged after 0.27.1 release.

@bcardiff bcardiff added this to the 0.28.0 milestone Nov 13, 2018
@bcardiff
Copy link
Member

@Sija do you want to update this with the || raise "..." idiom?

@Sija
Copy link
Contributor Author

Sija commented Nov 16, 2018

@bcardiff first I'd like to discuss adding a raising variant and I'll do it after that if there won't be a consensus.

@bcardiff
Copy link
Member

bcardiff commented Nov 16, 2018

I thought the consensus was that a single variant that cover all the cases was enough.

@Sija
Copy link
Contributor Author

Sija commented Nov 16, 2018

@bcardiff I don't think it's settled since as I wrote in #6967 (comment) I believe having raising one is still more beneficial - raising outside of read_file will obscure the actual error - which might be other than missing file.

@woodruffw
Copy link
Contributor

I began work on a variant of read_file that raises here: #7094.

@Sija Sija force-pushed the use-read_file-macro branch from 79f2924 to 2549629 Compare November 22, 2018 12:20
@Sija
Copy link
Contributor Author

Sija commented Nov 22, 2018

Since #7094 has been merged, it seems it's ready to 🎲 (once 0.27.1 is released).

Copy link
Member

@sdogruyol sdogruyol 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 @Sija 👍

@bcardiff
Copy link
Member

bcardiff commented Feb 2, 2019

@Sija would you mind pushing a rebase on master?

@Sija Sija force-pushed the use-read_file-macro branch from 2549629 to 2ced82b Compare February 2, 2019 04:05
@Sija
Copy link
Contributor Author

Sija commented Feb 2, 2019

@bcardiff Sure, done.

@bcardiff bcardiff merged commit 0da24ec into crystal-lang:master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants