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

Revert "Added InputFileParameter and OutputFileParameter classes." #1803

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

Tarrasch
Copy link
Contributor

@Tarrasch Tarrasch commented Aug 2, 2016

Reverts #1801

@mention-bot
Copy link

@Tarrasch, thanks for your PR! By analyzing the annotation information on this pull request, we identified @brcopeland, @mikekap and @dlstadther to be potential reviewers

@brcopeland
Copy link
Contributor

I suppose these could be problematic for systems that don't have access to the same files. Personally though I have found them helpful to reduce frequent boilerplate code. Anything occur to you to make it safer, or is something like this just not something you think should be a standard feature?

@Tarrasch Tarrasch merged commit 8545d6e into master Aug 4, 2016
@Tarrasch Tarrasch deleted the revert-1801-file_parameters branch August 4, 2016 03:24
@Tarrasch
Copy link
Contributor Author

Tarrasch commented Aug 4, 2016

I revert for now. But I'm still as open as over to merging an improved version of this. In addition to my comments in #1801, I think we need to introduce some mechanism in addition to Parameter.parse(), something like Parameter.parse_from_command_line. The idea is that it has a default implementation to just call out to parse. Meanwhile parse may only perform pure logic prase_from_command_line could do all kinds of conveniences for the user, in this case, it could convert a relative path to an absolute path.

Technically, this call to parse should be call parse_from_command_line instead.

As a practical advice to I would first focus on getting your other two less controversial Parameter-related patches merged first and then continue on this. :)

This was referenced Jun 29, 2022
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.

3 participants