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

Refactor/refactor classes #6

Merged
merged 37 commits into from
Sep 12, 2013

Conversation

thelightcosine
Copy link
Contributor

Large Refactor of this library. Split classes/modules out into their own seperate file structure
Small bits of cleanup for obviously wrong behaviour
Added lots of tests (now 681 SPECS and counting)
Fixed rake tasks to do rspec and simplecov correctly

Still TODO:
Add more test coverage
Add and clenaup YARD documentation
Add YARD rake task
refactor a few more wonky things once test coverage is in place

David Maloney added 30 commits July 23, 2013 13:54
first step, move classes into seperate files
to allow easier cleanup and refactoring
Circular Dependency between FieldSet and SecurityBuffer
load-order issues resolved
cleanup rake task to simple standard approach
start writing specs for base Field class
Look into shared example groups
This will make Field sublcasses easier
to test
Int16LE, Int32LE, and Int64LE now have
a shared example group. The behaviour is the same
the values just change.
Much cleaner tests this way.
Keeps you from making a bonehead mistake
Mostly jsut makes coverage numbers look better.
probably need to revisit later for other
test cases, but this works for now
still need to test instance methods
Shouldn't have more than 1 assertion
per it block. This works much better.
probably some more to wrap here,
but this gets the basics down for now
FieldSet and Message are a little
odd as they don't have fields themselves
but as they don't directly affect anything
we can kind of kludge around that for now.
This self-modifying class stuff seems unnecissary.
one spec failing. committing before i try
to fix the behaviour
fixing that test breaks others, leave it for now
Got a ways to go, but should be
able to get all the messages speced out
with this soon
some wierd inconsistencies between types
here. Also confused by what some of these
methods are trying to do
Get code coverage working with simplecov
existing message specs mvoed to the right
spec files.
Got pulled off this to do other things. add latest fix
and bump version.
Need to come back and finish adding tests, and fix up YARD
documentation later.
David Maloney added 2 commits August 16, 2013 10:56
pry was accidentally left in Gemfile
it { should respond_to :has_flag? }
it { should respond_to :set_flag }
it { should respond_to :dump_flags }
it { should respond_to :has_flag? }
Copy link

Choose a reason for hiding this comment

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

Dup of line 10

David Maloney added 5 commits August 16, 2013 11:15
fix few things pointed out by egypt
Some unicode issues that come up in ruby 2.0 and rbx
use pack to derive propper values rather
than use string literals. Eliminates wierd encoding
issues
rubinius has issues with this routine.
Force encoding the passed string as UTF-16
then do our conversion method.
apparently breaks 1.9.2
i hate charachter encoding!
@thelightcosine
Copy link
Contributor Author

hellloooooo?

@pmorton
Copy link
Contributor

pmorton commented Aug 25, 2013

Sorry. It's been a bust month. I will see if I can review this on Monday for impact. So far I am happy with what I see, though I need to do some manual testing to be sure that it does not break things. I am inclined to merge and bump if the manual testing works out.

@thelightcosine
Copy link
Contributor Author

heh , no worries. cheers!

@pmorton
Copy link
Contributor

pmorton commented Sep 12, 2013

@dmaloney-r7 I am stumbling a bit here on the integration testing. I am able to test that local authentication still works with httpclient, however I am not able to verify the same for domain authentication. Have you run this test case?

@pmorton
Copy link
Contributor

pmorton commented Sep 12, 2013

Okay. I figured it out... The domain ALWAYS has to be in caps. I will fix. Other than that, the merge looks good

pmorton added a commit that referenced this pull request Sep 12, 2013
@pmorton pmorton merged commit 6e6dc0a into WinRb:master Sep 12, 2013
@thelightcosine
Copy link
Contributor Author

awesome, thanks

@pmorton
Copy link
Contributor

pmorton commented Sep 12, 2013

This has been released at 0.4.0. You rock. 🚀

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