This repository has been archived by the owner. It is now read-only.

add changed sections / case numbers to commit messages #11

Merged
merged 3 commits into from Nov 4, 2013

Conversation

Projects
None yet
2 participants
@misjoinder
Contributor

misjoinder commented Oct 17, 2013

Sample output to Git commit: "FISC docket updated Case No. Misc. 13-04, Case No. Misc. 13-03"

  • Adds xml-simple gem
  • Adds Mac .DS_Store files to .gitignore
@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Oct 17, 2013

Owner

Hey, nice! I'd like to merge this, but a couple things first -

  • Could you move this code to a new file, and load it via require? I'd like to keep the main script small, and focused on the core fetch/diff/alert workflow.
  • I don't think this got tested under adverse conditions - if there's an exception, message won't be defined come commit-time, which means the whole thing will crash. Ensure the code works, and the commit will get made with a generic commit message, even if the whole thing blows up.
Owner

konklone commented Oct 17, 2013

Hey, nice! I'd like to merge this, but a couple things first -

  • Could you move this code to a new file, and load it via require? I'd like to keep the main script small, and focused on the core fetch/diff/alert workflow.
  • I don't think this got tested under adverse conditions - if there's an exception, message won't be defined come commit-time, which means the whole thing will crash. Ensure the code works, and the commit will get made with a generic commit message, even if the whole thing blows up.
@misjoinder

This comment has been minimized.

Show comment
Hide comment
@misjoinder

misjoinder Oct 17, 2013

Contributor

I didn't get to test this locally, but would this commit solve both issues?

Contributor

misjoinder commented Oct 17, 2013

I didn't get to test this locally, but would this commit solve both issues?

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Oct 17, 2013

Owner

It's close - Ruby 1.9 requires giving a path to a local require (even just ./ in front of it), and there's a redundant assignment of message.

And one more thing, sorry - could you surround this feature in a conditional, that is turned on by adding an option to the config file? This makes the merge safer, so that the new code won't run on my production server until I tell it to. (This is an artifact of how the project uses git to version fisa.html, which necessitates an automatic git pull of its own code every time it runs. I know that's unusual and weird, but it was the quickest way to do this.)

Owner

konklone commented Oct 17, 2013

It's close - Ruby 1.9 requires giving a path to a local require (even just ./ in front of it), and there's a redundant assignment of message.

And one more thing, sorry - could you surround this feature in a conditional, that is turned on by adding an option to the config file? This makes the merge safer, so that the new code won't run on my production server until I tell it to. (This is an artifact of how the project uses git to version fisa.html, which necessitates an automatic git pull of its own code every time it runs. I know that's unusual and weird, but it was the quickest way to do this.)

@misjoinder

This comment has been minimized.

Show comment
Hide comment
@misjoinder

misjoinder Oct 17, 2013

Contributor

ok - customization of Git commit message, the start of the custom message, and the fallback message are now set in the config

Contributor

misjoinder commented Oct 17, 2013

ok - customization of Git commit message, the start of the custom message, and the fallback message are now set in the config

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Oct 17, 2013

Owner

Great, thank you for doing this! I'll merge this on the weekend (when the Court is likely to do things and I can test thoroughly locally, and when I am more free). I think your change detection lib can be expanded to do a bunch of things, this is a great foundation.

Owner

konklone commented Oct 17, 2013

Great, thank you for doing this! I'll merge this on the weekend (when the Court is likely to do things and I can test thoroughly locally, and when I am more free). I think your change detection lib can be expanded to do a bunch of things, this is a great foundation.

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Oct 22, 2013

Owner

I haven't forgotten about this, just didn't get the chance I wanted. But I have parsing plans for this now, so I'll review this soon.

Owner

konklone commented Oct 22, 2013

I haven't forgotten about this, just didn't get the chance I wanted. But I have parsing plans for this now, so I'll review this soon.

@konklone konklone merged commit 70099c7 into konklone:master Nov 4, 2013

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Nov 4, 2013

Owner

So, I merged your changes in, and reworked them a bit into dockets.rb, which only concerns itself with returning an array of affected section names. Then, fisa.rb uses that to affect the commit message if needed. I also removed the config options, and made this execute by default, but wrapped it in a generic exception handler both inside and outside the call to Docket#changed (which is probably overkill).

Unfortunately, despite my best efforts, I don't appear to have preserved your author name on the commits. I don't really understand git well enough to know why, I thought me pulling down changes from your origin would ensure you at least got author credit. My apologies for that, I'll do a better job if you submit any more PRs from here on out.

The script is becoming a rat's nest to test with, so I'm working now on moving fisa.html into a separate branch, and making it so the script expects that branch to be checked out in some separate directory, pointed to in the config file. The branch and directory will be configured in config.yml. This way, the script isn't operating in its own directory, which is insanely chaotic and makes testing even harder.

Thanks again for doing this, and sorry it took me so long to take care of this -- I take the code's reliability very seriously, since the project has a spotlight on it, and I needed a block of time I could really focus on refactoring the project.

Owner

konklone commented Nov 4, 2013

So, I merged your changes in, and reworked them a bit into dockets.rb, which only concerns itself with returning an array of affected section names. Then, fisa.rb uses that to affect the commit message if needed. I also removed the config options, and made this execute by default, but wrapped it in a generic exception handler both inside and outside the call to Docket#changed (which is probably overkill).

Unfortunately, despite my best efforts, I don't appear to have preserved your author name on the commits. I don't really understand git well enough to know why, I thought me pulling down changes from your origin would ensure you at least got author credit. My apologies for that, I'll do a better job if you submit any more PRs from here on out.

The script is becoming a rat's nest to test with, so I'm working now on moving fisa.html into a separate branch, and making it so the script expects that branch to be checked out in some separate directory, pointed to in the config file. The branch and directory will be configured in config.yml. This way, the script isn't operating in its own directory, which is insanely chaotic and makes testing even harder.

Thanks again for doing this, and sorry it took me so long to take care of this -- I take the code's reliability very seriously, since the project has a spotlight on it, and I needed a block of time I could really focus on refactoring the project.

@misjoinder

This comment has been minimized.

Show comment
Hide comment
@misjoinder

misjoinder Nov 5, 2013

Contributor

Thanks! It's great to take part in this project.

I noticed you're adding a Dockerfile to the repo as well... I'm in the middle of setting one up for GlobaLeaks. I'll try your Dockerfile and let you know if I come up with any ideas.

Contributor

misjoinder commented Nov 5, 2013

Thanks! It's great to take part in this project.

I noticed you're adding a Dockerfile to the repo as well... I'm in the middle of setting one up for GlobaLeaks. I'll try your Dockerfile and let you know if I come up with any ideas.

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Nov 5, 2013

Owner

It's my first ever Dockerfile, so I can't promise that it's a good model! But that's really cool, I'd love to know how it goes.

Owner

konklone commented Nov 5, 2013

It's my first ever Dockerfile, so I can't promise that it's a good model! But that's really cool, I'd love to know how it goes.

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Nov 8, 2013

Owner

Here's the first commit using your work: ef50475

Owner

konklone commented Nov 8, 2013

Here's the first commit using your work: ef50475

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Nov 8, 2013

Owner

So @msjoinder, here's an unconsidered case: a new docket was added today, and the docket detection doesn't consider that possibility. Also, it somehow chose the docket beneath the added one, not the one above it as I would expect.

Owner

konklone commented Nov 8, 2013

So @msjoinder, here's an unconsidered case: a new docket was added today, and the docket detection doesn't consider that possibility. Also, it somehow chose the docket beneath the added one, not the one above it as I would expect.

@misjoinder

This comment has been minimized.

Show comment
Hide comment
@misjoinder

misjoinder Nov 8, 2013

Contributor

oh, I was hoping that it would detect the new header section

I'll take a look at this today and try to send up a patch today or tomorrow

Contributor

misjoinder commented Nov 8, 2013

oh, I was hoping that it would detect the new header section

I'll take a look at this today and try to send up a patch today or tomorrow

@misjoinder

This comment has been minimized.

Show comment
Hide comment
@misjoinder

misjoinder Nov 9, 2013

Contributor

I'm testing the docket.rb part on its own, with the same diff, and I get the new docket name (Case No. Misc. 13-08)

Because the complete version returned the next docket instead of the previous docket, I think that git.diff is returning the correct diff and line numbers, but File.open("fisa.html") in docket.rb must be reading from an older fisa.html. I can't test it locally, but maybe change File.open to the same open("#{@docket}/fisa.html", "wt") do |file| which is working in the master branch?

Contributor

misjoinder commented Nov 9, 2013

I'm testing the docket.rb part on its own, with the same diff, and I get the new docket name (Case No. Misc. 13-08)

Because the complete version returned the next docket instead of the previous docket, I think that git.diff is returning the correct diff and line numbers, but File.open("fisa.html") in docket.rb must be reading from an older fisa.html. I can't test it locally, but maybe change File.open to the same open("#{@docket}/fisa.html", "wt") do |file| which is working in the master branch?

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Nov 9, 2013

Owner

Yes - you nailed it. I had forgotten to pass in the @docket variable to dockets.rb, so it was looking for a local fisa.html. In this case, I'd forgotten to delete the old fisa.html from the server (or forgotten to pull the commit that git rm'd it, looks like).

Because of the aggressive exception handling I added around dockets.rb, it wasn't spitting out any errors locally. I fixed the underlying issue, and also made it so that upon any exception, the exception will at least get spat out to the console so it doesn't get totally ignored.

Owner

konklone commented Nov 9, 2013

Yes - you nailed it. I had forgotten to pass in the @docket variable to dockets.rb, so it was looking for a local fisa.html. In this case, I'd forgotten to delete the old fisa.html from the server (or forgotten to pull the commit that git rm'd it, looks like).

Because of the aggressive exception handling I added around dockets.rb, it wasn't spitting out any errors locally. I fixed the underlying issue, and also made it so that upon any exception, the exception will at least get spat out to the console so it doesn't get totally ignored.

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