-
Notifications
You must be signed in to change notification settings - Fork 12
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
Give more freedom when configuring Zonemaster::LDNS #134
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b6a937e
Options to provide OpenSSL distinct inc/lib paths
97576ce
Pass custom OpenSSL paths to LDNS C compiler
a29ed65
Add POD for optional features
9e46c8d
Update DNS data used in tests
9d23ca9
Editorial updates
156cca2
Fix default value in documentation
580888f
Document new OpenSSL options in README
f39abb9
Update documentation
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should it be "and" instead of "or" on line 177 (updated text) to match the statement on line 190-191 (updated text):
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 didn't want to restrict the usage of the options. One could use them separately. Hence the "or" line 177. About the documentation line 190-191, I think the meaning is also correct. You need both options to "specify both paths", hence the "and". Are you saying that this is confusing?
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 think the text in 189-191 says that both should be set if not headers and libraries are in the same directory.
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.
Which is how I expect people to use such options. But I again I didn't want to restrict the usage. The line 177 is the documentation of the following line in Makefile.PL:
zonemaster-ldns/Makefile.PL
Line 97 in f39abb9
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 think you say "and" above but "or" in the documentation.
You have added a lot of POD docuementation into Makefile.PL, which we do not have in other repositories. The documentation overlaps with the documentation in the README.md file. Wouldn't it be better to put the documentation in one place and then refer from the other?
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.
Looking at other repositories, I don't think we provide arguments to pass options to Makefile.PL.
I guess this would be better indeed. I'd say it would be better to have it close to the code so that you can refer to it easily without needing several files. And you can run
perldoc Makefile.PL
to access the documentation.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 think it would be fine to state "run
perldoc Makefile.PL
" in README.md.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'm not sure about removing the documentation from the README. And on another hand I think it's nice to provide POD for the Makefile.PL file. Can't we try to keep both documentation for now?
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.
If there are no conflicts between them there is at least no short-term harm. So that is OK.