Skip to content

util: Use ArgsManager::GetPathArg more widely#24675

Merged
fanquake merged 3 commits intobitcoin:masterfrom
hebasto:220325-path
Aug 4, 2022
Merged

util: Use ArgsManager::GetPathArg more widely#24675
fanquake merged 3 commits intobitcoin:masterfrom
hebasto:220325-path

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Mar 25, 2022

This PR is a continuation of #24265 and #24306.

Now the following command-line arguments / configure options been read with the GetPathArg method:

  • -conf, also includeconf values been normalized
  • -rpccookiefile

@hebasto hebasto mentioned this pull request Mar 26, 2022
Merged
@hebasto
Copy link
Member Author

hebasto commented Apr 4, 2022

Friendly ping @ryanofsky @prusnak

@laanwj
Copy link
Member

laanwj commented Apr 6, 2022

Concept ACK

@jarolrod
Copy link
Contributor

jarolrod commented Apr 8, 2022

concept ack

@DrahtBot DrahtBot mentioned this pull request Apr 9, 2022
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 10, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Apr 10, 2022
Closed
ryanofsky
ryanofsky approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 92aacda, but it would be good to consider suggestions related to the NormalizedPathFromString function.

@hebasto
Copy link
Member Author

hebasto commented Apr 21, 2022

@ryanofsky

Thank you for your review! Agree with all of points you mentioned above.

Updated.

@DrahtBot DrahtBot mentioned this pull request Jun 8, 2022
Closed
jarolrod
jarolrod reviewed Aug 2, 2022
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Code Review ACK b01f336

This is a nice cleanup and it is correct to expand the usage of GetPathArg to conf settings that are being passed a path, obviously :)

ryanofsky
ryanofsky approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK b01f336. Changes since last review: just dropping first commit (NormalizedPathFromString) as suggested

@fanquake fanquake merged commit e09ad28 into bitcoin:master Aug 4, 2022
@hebasto hebasto deleted the 220325-path branch August 4, 2022 16:12
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2022
@fanquake
Merge bitcoin#24675: util: Use ArgsManager::GetPathArg more widely
bef507a
@bitcoin bitcoin locked and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@jarolrod jarolrod jarolrod left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hebasto @laanwj @jarolrod @DrahtBot @ryanofsky @fanquake @maflcko