Skip to content

Introduce GetFileArg and use it where possible#24274

Closed
prusnak wants to merge 2 commits intobitcoin:masterfrom
prusnak:GetFileArg
Closed

Introduce GetFileArg and use it where possible#24274
prusnak wants to merge 2 commits intobitcoin:masterfrom
prusnak:GetFileArg

Conversation

@prusnak
Copy link
Contributor

@prusnak prusnak commented Feb 5, 2022 •
edited
Loading

Follow-up to #24265

Marking as draft until #24265 is merged as this PR builds on top of that PR

@prusnak prusnak mentioned this pull request Feb 5, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 5, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24266 (util: Avoid buggy std::filesystem:::create_directories() call by hebasto)
  • #23732 (refactor: Remove gArgs from bdb.h and sqlite.h by kiminuo)
  • #17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
  • #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
  • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
  • #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
  • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Feb 5, 2022
Closed
Merged
Merged
Draft
Draft
Draft
Draft
Draft
kiminuo
kiminuo reviewed Feb 6, 2022
View reviewed changes
@prusnak prusnak marked this pull request as ready for review February 9, 2022 21:56
@ryanofsky ryanofsky mentioned this pull request Feb 9, 2022
Merged
@ryanofsky
Copy link
Contributor

ryanofsky commented Feb 9, 2022 •
edited
Loading

Concept ACK, but I think PR description should be clearer. Also, I think it would be better to drop the first commit 4268810 here, and substitute commit b00c0c2 from #24306. Main reason is that I think it is confusing to have separate GetPathArg and GetFileArg methods which are exactly the same except one strips trailing slashes and one doesn't. I don't think it's likely we will have a need for the one which preserves trailing slashes. The commit from the other PR also adds unit test coverage.

@prusnak
Copy link
Contributor Author

prusnak commented Feb 10, 2022

Let's close this in favor of #24306

@prusnak prusnak closed this Feb 10, 2022
@prusnak prusnak deleted the GetFileArg branch February 10, 2022 22:27
maflcko pushed a commit that referenced this pull request Mar 7, 2022
Merge #24306: util: Make ArgsManager::GetPathArg more widely usable
6687bb2 
60aa179 Use GetPathArg where possible (Pavol Rusnak)
5b946ed util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky)
687e655 util: Add GetPathArg default path argument (Ryan Ofsky)

Pull request description:

  Improve `ArgsManager::GetPathArg` method added in recent PR #24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for #24274 which is parsing more path options.

  - Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values.
  - Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated.
  - Add unit tests for default and negated cases
  - Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable.

ACKs for top commit:
  w0xlt:
    Tested ACK 60aa179 on Ubuntu 21.10
  hebasto:
    re-ACK 60aa179

Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd
@bitcoin bitcoin locked and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@kiminuo kiminuo kiminuo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Utils/log/libs Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@prusnak @DrahtBot @ryanofsky @kiminuo