builder: impose argument naming consistency #212
Conversation
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
mathstuf
force-pushed
the
builder-arg-name-consistency
branch
2 times, most recently
from
74ced82 to
ce34662
Compare
|
Will rebase on #211 once merged. |
|
This is a great idea, thanks for this! I'd be fine using deprecation to rename the method, although nothing jumps to mind per se :( |
|
|
|
👍 |
Instead of `path` being the in-archive name in some arguments and the on-disk name in others, make it so that `name` is always the in-archive name and `path` is always an on-disk name.
This makes the method use the same "name comes first" argument order consistency used in all other methods. Deprecate `append_path_with_name` in favor of it.
mathstuf
force-pushed
the
builder-arg-name-consistency
branch
from
ce34662 to
d23dc27
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Instead of
pathbeing the in-archive name in some arguments and theon-disk name in others, make it so that
nameis always the in-archivename and
pathis always an on-disk name.It is unfortunate that
Builder::append_path_with_nameis the only one where thenameargument is not before what is being appended like every other method. Is there a new method name that could be used and the current one deprecated to keep consistency there as well. Is it worth it?