search

puppetlabs / ruby-hocon

A Ruby port of the Typesafe Config library.
Apache License 2.0
34 stars 30 forks source link

(MAINT) Prepare for 1.2.5 release #106

Closed jpinsonault closed 7 years ago

jpinsonault commented 7 years ago

Update changelog and version number

jpinsonault commented 7 years ago

ping @highb or @mwbutcher for review

rlinehan commented 7 years ago

Does the gemspec need to be updated to say it has a dependency on addressable now?

jpinsonault commented 7 years ago

It's in https://github.com/puppetlabs/ruby-hocon/blob/master/Gemfile

I think it's the first dependency this project has.

But I'm not good at ruby

highb commented 7 years ago

@rlinehan Oh huh, yeah I think it does need to be added to the gemspec, since that's what is actually looked at when building the gem, right?

rlinehan commented 7 years ago

I think so???? But I'm bad at Ruby.

Maybe this is too late, but is there any way we could solve the previous problem without requiring a new gem? Because probably we should go through the whole release new library process unless we're shipping it somewhere else.

rlinehan commented 7 years ago

(I didn't catch on that we didn't previously depend on addressable, sorry)

highb commented 7 years ago

Puppet appears to solve the issue without Addressable, but it seems like it would be better in the long term to use a widely-used gem that deals correctly with IRIs and URIs. Also, it doesn't seem great to copypasta whatever Puppet is doing. But if that is easier than dealing with release/licensing, maybe that's the way to go?

jpinsonault commented 7 years ago

I built the gem locally, installed it and ran gem dependency on it, and it doesn't list addressable as a gem.

I agree that adding a dependency for this one line change isn't ideal if it's going to introduce a whole new dependency for PE, but it's also a good gem that does what ruby's URI should do by default

jpinsonault commented 7 years ago

Well maybe not what URI should do, but either way, I'm not sure what's the better option here

jpinsonault commented 7 years ago

I'm looking at the code, and I'm not even sure why it needs to be a URI. The actual file loading doesn't even happen in the same class. It's been years since I was involved in the depths of hocon, but all the tests pass if I change SimpleConfigOrigin.new_file to just set the url variable to a string instead of a URI

jpinsonault commented 7 years ago

It opens the file here: https://github.com/puppetlabs/ruby-hocon/blob/master/lib/hocon/impl/parseable.rb#L362-L379

and if you trace the calls back up, it receives the unmodified string object that you pass into Hocon.load

jpinsonault commented 7 years ago

So an important thing I'd forgotten: We explicitly list loading URLs as an unsupported feature (java hocon supports loading from URLs, which... seems less than great, and I don't think we ever intend to support this feature)

It seems like the reason we parse the file paths as URIs is so parts of the codebase can examine it and see if it's a file or url. But the URL part is pointless since we don't care about URLs.

I think the only place the URI is used is here: https://github.com/puppetlabs/ruby-hocon/blob/master/lib/hocon/impl/simple_config_origin.rb#L156-L175

And that function is just to get the filename from the URI. If it's not a file URI, it returns nil. The URI is never used other than that.

So I propose we stop trying to parse it as a URI when we only ever expect it to be a file path. And maybe create a ticket to cleanup the code later.

Thoughts?

highb commented 7 years ago

@jpinsonault That seems OK for now, but if we want an ecosystem to actually use this gem, we probably want to support URI/IRI someday. :)

jpinsonault commented 7 years ago

I'm not so sure there's much demand for loading anything other than files (and strings), and I'm hesitant to support loading arbitrary URIs (specifically URLs) anyways. Is anyone asking for that? Totally open to changing my opinion, but either way it's not a supported feature right now, and getting addressable in here isn't going to move us towards that goal without some reasonably significant reworking of the code

It just seems like we can avoid this situation pretty easily, since hocon doesn't currently make use of any URIs. I can take care of the PR to fix this up

highb commented 7 years ago

Yeah, we're only concerned with loading file paths in the installer, so I'd be fine with just dropping the URI.

highb commented 7 years ago

As long as it works with unicode in the path, that is! :)

jpinsonault commented 7 years ago

Awesome, and sorry for run around. I wasn't able to put a lot of thought into this this week

jpinsonault commented 7 years ago

Closing for now, I'll get a PR up soon