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

Included file path is based on including file directory #114

Closed
wants to merge 2 commits into from

Conversation

SeaTalk
Copy link
Contributor

@SeaTalk SeaTalk commented Jan 21, 2019

This change is motivated by incorrect outputs when I use cpp-hocon to parse our conf files.
U can find more details by clicking this link about what situation I ran into.
I wonder whether someone ever been where I am, and I think may be this change could also be a solution to them.

…d file path is either an absolut path or a relative path based on the path of a.conf.

Before this change, work-directory is the base path of included file.
@puppetcla
Copy link

Waiting for CLA signature by @SeaTalk

@SeaTalk - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html

@puppetcla
Copy link

CLA signed by all contributors.

@SeaTalk
Copy link
Contributor Author

SeaTalk commented Jan 22, 2019

No report from trvis-ci yet. Don't know what I am supposed to do. 🤔

@MikaelSmith
Copy link
Contributor

I'll check up on Travis CI.

@MikaelSmith
Copy link
Contributor

Our linter is complaining about a few things.

Copy link
Contributor

@MikaelSmith MikaelSmith left a comment

Choose a reason for hiding this comment

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

I left some pretty high-level comments. Do you have an interest in trying to improve this, or should we consider this an open problem for someone to build on your solution?

@@ -9,6 +9,31 @@ This is a port of the TypesafeConfig library to C++.

The library provides C++ support for the [HOCON configuration file format](https://github.com/typesafehub/config/blob/master/HOCON.md).

## I Made Some Change
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably makes more sense as the commit message rather than part of the README.

@@ -0,0 +1,26 @@
#include <hocon/parser/config_document_factory.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to the lib/test directory and integrated with our test suite.

@@ -0,0 +1,10 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

All of example/conf and imgs/ seem like they should be part of the test suite as well.

if (nullptr == fpath) {
fpath = make_shared<full_path_operator>();
}
extract_filename_from_path(file_basename, &file_dir, &file_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

My instinct is that this isn't the place we should be fixing this, but it'd take some work for me to follow up on.

From a quick glance at https://github.com/lightbend/config/blob/7b9e1539dd46385e9b83be43c4112a448129ac2f/config/src/main/java/com/typesafe/config/impl/Parseable.java#L639-L655, I think the mistake may be in not overriding relative_to on parseable_file (and possibly others) in https://github.com/puppetlabs/cpp-hocon/blob/master/lib/src/parseable.cc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left some pretty high-level comments. Do you have an interest in trying to improve this, or should we consider this an open problem for someone to build on your solution?

Thanks for code-reviewing and comments. I think we could left the problem open before solved. And I'll keep trying to figure another way out to fix it. Thanks again~
It's 3 in the morning and I need some sleep. Good day!

@MikaelSmith
Copy link
Contributor

Closed in favor of the other PR.

@MikaelSmith MikaelSmith closed this Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants