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

relative file path of a include statement relatives to directory of including file #115

Merged
merged 2 commits into from
Feb 1, 2019

Conversation

SeaTalk
Copy link
Contributor

@SeaTalk SeaTalk commented Jan 29, 2019

This change is another solution for #114

@SeaTalk SeaTalk changed the title relative file path of a include statement is relative to directory of including file relative file path of a include statement relatives to directory of including file Jan 29, 2019
@puppetcla
Copy link

CLA signed by all contributors.

@SeaTalk
Copy link
Contributor Author

SeaTalk commented Jan 29, 2019

Anyone can review this change ?

@MikaelSmith
Copy link
Contributor

This looks like it could work. Did you explore my idea in #114 (comment) and find it didn't help?

@MikaelSmith
Copy link
Contributor

This generally seems good. @caseywilliams interested in doing a review?

@SeaTalk
Copy link
Contributor Author

SeaTalk commented Jan 30, 2019

This looks like it could work. Did you explore my idea in #114 (comment) and find it didn't help?

I followed your idea to find out the key issue exists the isolation of context for each parseable instance. The parseable_file of included file has not any information from including file. I didn't override function "relative_to" for sub-classes of parseable since we don't have a particular path for "resource", like java, in c++ for now.

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.

Looks good generally, just a few minor comments.

lib/tests/simple_confs/a.conf Outdated Show resolved Hide resolved
@@ -105,17 +105,38 @@ namespace hocon {
// if it starts with "/" then remove the "/", for consistency
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this comment is no longer accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'll try to correct it

…relative file path problem of include statements in hocon files.(A test case is added into conf_parser_test.cc)
@SeaTalk
Copy link
Contributor Author

SeaTalk commented Jan 31, 2019

@MikaelSmith I take your advises and modify a little. Take another glance please

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.

This looks good. I'd like to get another person reviewing it however. I'll work on that.

Copy link
Contributor

@Magisus Magisus left a comment

Choose a reason for hiding this comment

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

Except for one spelling thing, I think this looks okay. The test cases look thorough, so 👍

}
}

bool context_initialed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except for one spelling thing, I think this looks okay. The test cases look thorough, so 👍

Thanks for reviewing. I'll correct that ASAP.

@SeaTalk
Copy link
Contributor Author

SeaTalk commented Feb 1, 2019

Done. @MikaelSmith @Magisus

@MikaelSmith
Copy link
Contributor

Thanks so much for your work on this!

@MikaelSmith MikaelSmith merged commit 777b2d6 into puppetlabs:master Feb 1, 2019
@zzhang97
Copy link

I was caught by this bug too with latest release. Happily found the fix was already in master. Thanks!

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

5 participants