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.

Sign up for GitHub

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

Add read_file macro method #6967

Merged
merged 11 commits into from
Nov 8, 2018
Merged

Add read_file macro method #6967

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
91c8a3f
Add `read_file` macro method
Sija Oct 20, 2018
9200e38
Refactor run & read_file macros, as per @asterite's review
Sija Nov 4, 2018
0f801d7
Yield error message instead of Exception
Sija Nov 4, 2018
2f378a3
Pass return value of yielded block
Sija Nov 4, 2018
82fe311
Fixes as per @straight-shoota's review
Sija Nov 6, 2018
0820ae8
Don't use relative path resolution in 'read_file' macro
Sija Nov 7, 2018
f13cccb
Add NOTE and spec regarding relative paths
Sija Nov 7, 2018
a2d70e4
Use more apt File.file? check instead of File.exists?
Sija Nov 7, 2018
9aa5f09
Refactor filename resolution in `find_file` to be more consistent
Sija Nov 7, 2018
29adc35
Revert `run` macro refactors
Sija Nov 7, 2018
2ae9e53
Move `interpret_read_file` definition under lexicographically correct
Sija Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions spec/compiler/macro/macro_methods_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1757,4 +1757,34 @@ module Crystal
end
end
end

describe "read_file" do
context "with absolute path" do
it "reads file (exists)" do
run(%q<
{{read_file("#{__DIR__}/../data/build")}}
>, filename = __FILE__).to_string.should eq(File.read("#{__DIR__}/../data/build"))
end

it "reads file (doesn't exists)" do
run(%q<
{{read_file("#{__DIR__}/../data/build_foo")}} ? 10 : 20
>, filename = __FILE__).to_i.should eq(20)
end
end

context "with relative path" do
it "reads file (exists)" do
run(%q<
{{read_file("spec/compiler/data/build")}}
>, filename = __FILE__).to_string.should eq(File.read("spec/compiler/data/build"))
end

it "reads file (doesn't exists)" do
run(%q<
{{read_file("spec/compiler/data/build_foo")}} ? 10 : 20
>, filename = __FILE__).to_i.should eq(20)
end
end
end
end
13 changes: 13 additions & 0 deletions src/compiler/crystal/macros.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,19 @@ module Crystal::Macros
def raise(message) : NoReturn
end

# Reads a file: if it exists, returns a `StringLiteral` with its contents;
# otherwise `nil` is returned.
#
# To read a file relative to where the macro is defined, use:
Sija marked this conversation as resolved.
Show resolved Hide resolved
#
# ```
# read_file("#{__DIR__}/some_file.txt")
Copy link
Member

Choose a reason for hiding this comment

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

I think ./some_file.txt should work, can you try it out? The idea is that this works like require.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, it probably works. But I don't think it should. Relative paths should be relative to the current working directory.
Either way, there should be a spec for reading a relative path.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we don't do require "#{__DIR__}/...", we use relative paths. require can be thought as a macro call that requires the given file. read_file is another macro that works the same way but reads it at compile-time. Because it's compile-time, I think it makes sense to use the same rules as require. In fact, the whole change that @Sija made because of me requesting it is exactly that.

Copy link
Member

@straight-shoota straight-shoota Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The argument to require is always an (explicit) string literal. There is no reason to make it customizeable. And there is no reason to require a file relative to the working directory.

But to read_file you might want to pass a StringLiteral as argument that is originally defined in a completely different place. And it there are perfectly fine reasons to read a file relative to the current working directory instead of the source location.

So I don't think require and read_file should share the same path resolution.

# ```
#
# NOTE: Relative paths are resolved to the current working directory.
def read_file(filename) : StringLiteral | NilLiteral
end

# Compiles and execute a Crystal program and returns its output
# as a `MacroId`.
#
Expand Down
16 changes: 16 additions & 0 deletions src/compiler/crystal/macros/methods.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ module Crystal
interpret_system(node)
when "raise"
interpret_raise(node)
when "read_file"
interpret_read_file(node)
when "run"
interpret_run(node)
else
Expand Down Expand Up @@ -142,6 +144,20 @@ module Crystal
raise SkipMacroException.new(@str.to_s)
end

def interpret_read_file(node)
Sija marked this conversation as resolved.
Show resolved Hide resolved
unless node.args.size == 1
node.wrong_number_of_arguments "macro call 'read_file'", node.args.size, 1
end

node.args[0].accept self
filename = @last.to_macro_id
if File.file?(filename)
@last = StringLiteral.new(File.read(filename))
else
@last = NilLiteral.new
end
end

def interpret_system(node)
cmd = node.args.map do |arg|
arg.accept self
Expand Down