Page MenuHomePhabricator

fix: add some arguments to facilitate extensions
AbandonedPublic

Authored by hooper on Jan 7 2019, 6:04 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

For example, these additions allow wrapping hg fix to do things like:

  • Telling fixer tools what commit a file belongs to
  • Cleaning up temporary files created by Fixer objects

There are no intended functional changes.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

hooper created this revision.Jan 7 2019, 6:04 PM
durin42 added a subscriber: durin42.Jan 7 2019, 6:54 PM

I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?

hooper added a comment.Jan 7 2019, 7:12 PM

I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?

Google's internal use of this extension makes some customizations that I doubt would be appealing in core. This patch makes some of it simpler to implement. Making all of it possible through configs/templates might be unduly complex.

A better version of this might put "ctx" into the templater inside Fixer.command, and maybe add a no-op Fixer.cleanup method to make that part more explicit. I think that would be a less trivial super set of this patch.

In D5516#81683, @hooper wrote:

I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?

Google's internal use of this extension makes some customizations that I doubt would be appealing in core. This patch makes some of it simpler to implement. Making all of it possible through configs/templates might be unduly complex.

What kinds of customizations?

A better version of this might put "ctx" into the templater inside Fixer.command, and maybe add a no-op Fixer.cleanup method to make that part more explicit. I think that would be a less trivial super set of this patch.

hooper added a comment.Jan 9 2019, 3:15 PM
In D5516#81683, @hooper wrote:

I'm -0 on this: what's special about this functionality that it needs to be an extension of an extension instead of something that can be done in core?

Google's internal use of this extension makes some customizations that I doubt would be appealing in core. This patch makes some of it simpler to implement. Making all of it possible through configs/templates might be unduly complex.

What kinds of customizations?

One is to aggregate metadata output from multiple fixer tool executions to display a summary at the end (so wrapping cleanup() is sensible). Not sure who else would use that, or if there's a good way to make a generic interface for it.

Another is basically to add a "--nodeid_for_this_file=deadbeef" to a fixer tool command line. That's where it would be sufficient to have the ctx available. It might be nice to put the ctx into the templater, but that raises some questions about implementation that I wanted to punt for now.

A better version of this might put "ctx" into the templater inside Fixer.command, and maybe add a no-op Fixer.cleanup method to make that part more explicit. I think that would be a less trivial super set of this patch.

hooper abandoned this revision.Mar 21 2019, 10:58 PM

See D6167 for a newer approach.