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

Issue-43: When --format is specified with the log method, return everyth... #57

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@estrabd

estrabd commented Jan 12, 2015

...ing.

This commit addresses Issue 43, which identifies the limitation in G::W that
there is no way to parse out each log entry for custom formats.

The solution is to provide a G::W::Log::Custom wrapper that returns all of
the output from the log method in the specified format with no attempt
to parse this output into individual records.

If the caller of the log method specifies the format, then they are responsible
for parsing the output.

There is the temptation here to all a parse subroutine reference to be passed,
but I would strongly encourage that we not get into the business of parsing
customized git log formats.

B. Estrade
Issue-43: When --format is specified with the log method, return ever…
…ything.

This commit addresses Issue 43, which identifies the limitation in G::W that
there is no way to parse out each log entry for custom formats.

The solution is to provide a G::W::Log::Custom wrapper that returns all of
the output from the log method in the specified format with no attempt
to parse this output into individual records.

If the caller of the log method specifies the format, then they are responsible
for parsing the output.

There is the tempation here to all a parse subroutine reference to be passed,
but I would strongly encourage that we not get into the business of parsing
customized git log formats.

@estrabd estrabd referenced this pull request Jan 12, 2015

Closed

git log --format #43

@genehack

This comment has been minimized.

Show comment
Hide comment
@genehack

genehack Feb 24, 2015

Owner

I'm still thinking about whether the ::Custom class is needed or not.

Owner

genehack commented Feb 24, 2015

I'm still thinking about whether the ::Custom class is needed or not.

@estrabd

This comment has been minimized.

Show comment
Hide comment
@estrabd

estrabd Feb 24, 2015

It was the cleanest solution I could think of. The alternative is to provide a way for individuals to supply their own "parse" function that that is consistent with whatever arbitrary format they provide for the output.

The ::Custom class allows them to just grab all of the output, then do something with it externally, which I think is the right call. This doesn't have to be a new class, but it should be possible to say "don't parse the output, just give it to me for post process".

estrabd commented Feb 24, 2015

It was the cleanest solution I could think of. The alternative is to provide a way for individuals to supply their own "parse" function that that is consistent with whatever arbitrary format they provide for the output.

The ::Custom class allows them to just grab all of the output, then do something with it externally, which I think is the right call. This doesn't have to be a new class, but it should be possible to say "don't parse the output, just give it to me for post process".

@genehack

This comment has been minimized.

Show comment
Hide comment
@genehack

genehack Mar 20, 2015

Owner

Here is what I think I'm going to do: if args includes a 'format', the return will just be the raw unparsed @out, but we'll also emit a warning that when using a specific format, the preferred invocation is to use the $git->RUN('log') method that's already documented.

@estrabd Does that seem okay?

Owner

genehack commented Mar 20, 2015

Here is what I think I'm going to do: if args includes a 'format', the return will just be the raw unparsed @out, but we'll also emit a warning that when using a specific format, the preferred invocation is to use the $git->RUN('log') method that's already documented.

@estrabd Does that seem okay?

@estrabd

This comment has been minimized.

Show comment
Hide comment
@estrabd

estrabd Mar 20, 2015

It's up to you. I would rather it throw an exception and die, pointing me to the documentation for RUN; maybe add a special section in the docs talking about "parsing custom log formats". I'd be happy to update the docs for that.

My reason for preferring a die on "format" is that without format, you're able to iterate over the parsed results. With format you're given a blob of unparsed text - it's then up to the caller to parse that blob into individual records if they wish. I'd rather get the same iterator every time I called log, and if I tried to get fancy with a format then a simple slap on the hand would set me right.

I do think that at the end of the day, your approach accomplishes what what I had done, only more directly.

Hope that helps.

Brett

estrabd commented Mar 20, 2015

It's up to you. I would rather it throw an exception and die, pointing me to the documentation for RUN; maybe add a special section in the docs talking about "parsing custom log formats". I'd be happy to update the docs for that.

My reason for preferring a die on "format" is that without format, you're able to iterate over the parsed results. With format you're given a blob of unparsed text - it's then up to the caller to parse that blob into individual records if they wish. I'd rather get the same iterator every time I called log, and if I tried to get fancy with a format then a simple slap on the hand would set me right.

I do think that at the end of the day, your approach accomplishes what what I had done, only more directly.

Hope that helps.

Brett

@genehack

This comment has been minimized.

Show comment
Hide comment
@genehack

genehack Mar 20, 2015

Owner

I was thinking about the exception throwing route too. Now that I've reviewed the whole entire saga across the original tickets, and I see that this has never worked properly, I'm okay with the exception being thrown instead of the warning. (I was worried I might be breaking working code; that seems unlikely based on the original ticket.)

I'll work on revising the PR, or if you wanted to do that, that would be great. 8^)

Owner

genehack commented Mar 20, 2015

I was thinking about the exception throwing route too. Now that I've reviewed the whole entire saga across the original tickets, and I see that this has never worked properly, I'm okay with the exception being thrown instead of the warning. (I was worried I might be breaking working code; that seems unlikely based on the original ticket.)

I'll work on revising the PR, or if you wanted to do that, that would be great. 8^)

@estrabd

This comment has been minimized.

Show comment
Hide comment
@estrabd

estrabd Mar 23, 2015

Sure, I can take a crack at it.

estrabd commented Mar 23, 2015

Sure, I can take a crack at it.

@genehack

This comment has been minimized.

Show comment
Hide comment
@genehack

genehack Mar 23, 2015

Owner

Thanks!

Owner

genehack commented Mar 23, 2015

Thanks!

@estrabd

This comment has been minimized.

Show comment
Hide comment
@estrabd

estrabd Mar 23, 2015

Closing in favor of discussed option.

estrabd commented Mar 23, 2015

Closing in favor of discussed option.

@estrabd estrabd closed this Mar 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment