-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add INI.build #5298
Add INI.build #5298
Conversation
c1e3b2c
to
077dd10
Compare
src/ini.cr
Outdated
io = IO::Memory.new | ||
ini.each do |key, value| | ||
io << "[" << key << "]\n" | ||
value.each do |str| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do .each do |key, value|
here and rename the existing key
to section
and the existing value
to contents
.
Oh and I just noticed that using single quotes over double quotes where you can ( |
@jhass Thanks for the tips, I've done the changes. |
src/ini.cr
Outdated
contents.each do |key, value| | ||
io << key << space << '=' << space << value << "\n" | ||
end | ||
io << "\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last one, promise, Crystal actually interprets \n
within single quotes since they're used to distinguish the type, not whether escape sequences are interpreted ;)
spec/std/ini_spec.cr
Outdated
|
||
describe "build from Hash(String, Hash(String, String)" do | ||
it "build file" do | ||
INI.build(INI.parse(File.read "#{__DIR__}/data/test_file.ini"), " ").should eq(File.read "#{__DIR__}/data/test_file.ini") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add an actual example in code which does not rely on the INI.parse
and the contents of an additional file. The example in the docstring would do.
src/ini.cr
Outdated
# ``` | ||
# INI.build({"foo" => {"a" => "1"}}, " ") # => "[foo]\na = 1\n\n" | ||
# ``` | ||
def self.build(ini : Hash(String, Hash(String, String)), space : String = "") : String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a configurable space
really useful? And why is it not a single whitespace by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do this because there are INI files with and without spaces around =
. It's not clear what is the good default. For example, systemd services has without spaces and php.ini
has one space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space
should be IMO " "
(space!) from default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method (and the overload below) should also take NamedTuple(String, String)
as an ini argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, removing the type restriction from the method signature, the code should instantly work for named tuples and other payload types than String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add Boolean, Number, String
according to the different implementations listed in Wikipedia. Array
isn't always supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just remove all type restrictions. This way, you can pass in any type and the builder just prints the result of to_s
. This is the most developer-friendly solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@straight-shoota I know, but adding NamedTuple(String, String)
raises can only instantiate NamedTuple with named arguments
.
By the way regardless of the types, the mapping should follow the Key(Value, Key(Key, Value)
structure.
That's OK, we will just need to add verifications and raise custom errors to avoid incorrect INI files if we accept everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do it like this:
module INI
def self.build(io : IO, ini, space : String = "")
ini.each do |section, contents|
io << '[' << section << "]\n"
contents.each do |key, value|
io << key << space << '=' << space << value << '\n'
end
io.puts
end
end
end
INI.build(STDOUT, {"main": {foo: "bar", baz: 12, regex: /regex/}})
No need for type restrictions or custom errors. If a data structure that does not provide a matching interface, it will automatically result in a compile time error.
src/ini.cr
Outdated
# INI.build({"foo" => {"a" => "1"}}, " ") # => "[foo]\na = 1\n\n" | ||
# ``` | ||
def self.build(ini : Hash(String, Hash(String, String)), space : String = "") : String | ||
io = IO::Memory.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use a String::Builder
instead of Memory::IO
, because this is optimized for creating a string.
And please add a method overload INI.build(io : IO, hash)
that can directly write to an IO instead of allocating a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've first found IO::Memory.new
if the Crystal performance guide. So we can further improve perfomance using String::Builder
for strings; I will make a PR for this, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not found yet a case when IO::Memory.new
is slower than the String.build
approach, using Benchmark.ips
. Anyone has a sample to demonstrate this? We will be able to add it to the Performance Guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require "benchmark"
Benchmark.ips do |x|
x.report("String.build") do
String.build do |io|
100.times do
io << "hello world"
end
end
end
x.report("IO::Memory") do
io = IO::Memory.new
100.times do
io << "hello world"
end
io.to_s
end
end
Output:
String.build 475.55k ( 2.1µs) (± 1.25%) fastest
IO::Memory 252.7k ( 3.96µs) (± 1.47%) 1.88× slower
whats your benchmark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/ini.cr
Outdated
contents.each do |key, value| | ||
io << key << space << '=' << space << value << '\n' | ||
end | ||
io << '\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line could be replaced by io.puts
e1363cd
to
b16aef1
Compare
src/ini.cr
Outdated
# INI.build({"foo" => {"a" => "1"}}, " ") # => "[foo]\na = 1\n\n" | ||
# ``` | ||
def self.build(ini : Hash(String, Hash(String, String)), space : String = "") : String | ||
str = String::Builder.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use String.build
instead of instantiating a builder directly.
src/ini.cr
Outdated
# ``` | ||
def self.build(ini : Hash(String, Hash(String, String)), space : String = "") : String | ||
String.build do |str| | ||
build str, ini, " " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last argument should be space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, miss this one
src/ini.cr
Outdated
|
||
# Appends INI data to the given IO. | ||
# | ||
def self.build(io : IO, ini : Hash(String, Hash(String, String)), space : String = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
spec/std/ini_spec.cr
Outdated
@@ -1,6 +1,53 @@ | |||
require "spec" | |||
require "ini" | |||
|
|||
class INI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in the spec??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've pasted this by mistake. I revert this ASAP
@@ -1,8 +1,8 @@ | |||
[general] | |||
log_level = DEBUG | |||
log_level = D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better not to change existing test data. You don't really need to use this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's for Char
type testing, that should be converted to a String
.
src/ini.cr
Outdated
|
||
# Appends INI data to the given IO. | ||
# | ||
def self.build(io : IO, ini, space : String = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think that having the space be configurable as any string is a bit too much and makes it possible to create invalid INI files. Why not make it a boolean which switches between ""
and " "
. Regardless, I think 1 space should be the default, not zero. We can always change this default later. Actually nvm, let's maap whitespace false
by default.
src/ini.cr
Outdated
ini.each do |section, contents| | ||
io << '[' << section << "]\n" | ||
contents.each do |key, value| | ||
io << key << space << '=' << space << value << '\n' | ||
io << key << s << '=' << s << value << '\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather
io << key
io << ' ' if space
io << '='
io << ' ' if space
io << value << '\n'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done it with io << key << (space ? " = " : '=') << value << '\n'
, less verbose. Is it fine?
5d38bf3
to
6bf43c5
Compare
Please add some specs for |
src/ini.cr
Outdated
end | ||
|
||
# Appends INI data to the given IO. | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant blank line.
2030d85
to
e486db7
Compare
Implements a basic method to generate an INI-style configuration from a hash.