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

compiler: Introduce non-nilable read_file macro #7094

Merged
merged 4 commits into from Nov 22, 2018

Conversation

Projects
None yet
7 participants
@woodruffw
Copy link
Contributor

woodruffw commented Nov 19, 2018

Follow-up on #6967.

The original (nilable) macro has been renamed to read_file?.

compiler: Introduce non-nilable read_file macro
The original (nilable) macro has been renamed to `read_file?`.

woodruffw added some commits Nov 19, 2018

spec: Fix expected exception
Prior was with a bare raise in the compiler.
@last = StringLiteral.new(File.read(filename))
else
rescue e

This comment has been minimized.

@Sija

Sija Nov 19, 2018

Contributor

e -> ex plz

This comment has been minimized.

@yxhuvud

yxhuvud Nov 19, 2018

Contributor

Also, one question is if it is a good idea to rescue all possible errors instead of only the specific one that is raised if the file doesn't exist.

This comment has been minimized.

@Sija

Sija Nov 19, 2018

Contributor

@yxhuvud I don't see a problem with it since they're re-raised and the only meaningful call (which might raise) is File.read.

This comment has been minimized.

@woodruffw

woodruffw Nov 19, 2018

Author Contributor

and the only meaningful call (which might raise) is File.read

Yep, this was my reasoning here 🙂

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Nov 21, 2018

read_file should return nil. From my experience, all other macro methods return nil instead of raising in the "missing" case. Since the macro language is dynamically typed, returning nil instead of erroring is the better default.

I vote close this PR. the || raise is the correct way to do this in macro-world.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

woodruffw commented Nov 21, 2018

all other macro methods return nil instead of raising in the "missing" case

Two counterexamples: Both the system and run macros raise on their error cases (failed execution and missing file, respectively). I'd argue that read_file bears more resemblance in kind to these two macros than others, and so should raise like them.

Similarly, from the end user's perspective, I'd argue that raising an error is a better default: returning a nil literal converts an I/O failure into a type mismatch, or (worse!) silently does the wrong thing if the user only ever calls #to_s or any other quack-y calls. That's avoidable with the || raise workaround, but I think doing it by default is more programmer-friendly.

@ysbaddaden

This comment has been minimized.

Copy link
Member

ysbaddaden commented Nov 22, 2018

@RX14 the run macro raises if the executed program exits with a non zero status.

If I embed a file, I expect the file to exist and be readable. I want a compile time error with the actual reason if it failed to. I don't want to deal with a nilable or have to raise myself a redundant read_file("path/to/file.png") || raise "failed to embed path/to/file.png" when the macro just could do it and give me the actual reason.

@RX14

RX14 approved these changes Nov 22, 2018

Copy link
Member

RX14 left a comment

Oh, I thought system didn't raise. I guess this is fine then.

@RX14 RX14 merged commit f3a2c57 into crystal-lang:master Nov 22, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RX14 RX14 added this to the 0.27.1 milestone Nov 22, 2018

@woodruffw woodruffw deleted the woodruffw-forks:read_file_macro branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment