Skip to content
This repository has been archived by the owner on Nov 3, 2019. It is now read-only.

Issue #9 - Extending the IDbExecutor interface #10

Closed
wants to merge 3 commits into from

Conversation

PureKrome
Copy link

This PR is referring to Issue #9.

  • Added Open / Close methods to the IDbExecutor interface.
  • Implemented said new methods.
  • Updated version to 0.4.

Unfortunately I'm PR'ing this to master and not Dev cause there is no Dev branch on the upstream repo.

- Interface updated with Open and Close methods.
- SqlExecutor references the underlying SqlConnection for Open/Close.
- Added some Xml comments in various places.
@half-ogre
Copy link
Owner

Thanks @PureKrome! I'll have a look tomorrow.

Unfortunately I'm PR'ing this to master and not Dev cause there is no Dev branch on the upstream repo.

This is by design. Master is the only non-ephemeral branch in GitHub Flow.

<configuration>
<solution>
<add key="disableSourceControlIntegration" value="true" />
</solution>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I get for not having a .gitattributes file in this repo 😢. I'll fix this up after I merge it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't build the sln when i first cloned it. So i enabled NuGet package restore which put the nuget.exe in and (i think) updated one or both (existing) nuget.config/.targets files.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with you. But this file didn't actually change; it's only showing as changed because of line endings, because I haven't put a .gitattributes file in.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh Ha yeah. I miss read - thinking it said git IGNORE.

FML + git EOF crazyness.

@half-ogre
Copy link
Owner

@PureKrome: My only issue with this PR is committing nuget.exe. I'd really rather have it download as needed. I'll see if I can dig up a sample of doing that, or you could just add it to the .gitignore and I'll fix it up after I merge this. But you'll need to redo this commits, because I don't want the extra size in the refs.

@PureKrome
Copy link
Author

Sure. i can redo this .. but I have no idea how to undo my commit .. then re-add a fresh commit minus the .exe. I only ever do clone/pull/push/commit/branch-switch/tag.

FWIW, i couldn't build the repo Out of the Box. I opened up the sln and fail. That's why i pulled down the latest nuget (ie. right click solution -> enable pack restore).

As a testing guru (you, not me .. i've got far to learn 😁) - i'm assuming your a big fan of test isolation. When i see a sln like this, I would see each sln as an Isolated project, where all resources should not be dependant outside of this. Having the .exe outside of this sln is like saying - use nuget in the GAC (and boy I hate the GAC and the crap that's caused).

That's how I see it at least - but i'm a newbie with all this.

Would love to know your reasoning behind not wanting to have the exe in there. is it really just size at the cost of a PITA to build?

@half-ogre
Copy link
Owner

FWIW, i couldn't build the repo Out of the Box. I opened up the sln and fail. That's why i pulled down the latest nuget (ie. right click solution -> enable pack restore).

That's totally on me and not having had a project to actually use this on for ages. I'll get that fixed up in master; sorry!

Would love to know your reasoning behind not wanting to have the exe in there. is it really just size at the cost of a PITA to build?

It's not a PITA to build when it's set up right. It automatically downloads nuget.exe for CI, and VS doesn't need it anymore for package restore on the latest version of NuGet.

@PureKrome
Copy link
Author

It automatically downloads nuget.exe for CI, and VS doesn't need it anymore for package restore on the latest version of NuGet.

Really?

What magic is this, you speak of?

image

@PureKrome
Copy link
Author

Closing this PR because I keep seeing it listed in my Created Pull Requests list and we've all moved on from this.

@PureKrome PureKrome closed this Jul 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants