-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feature: tab stops #261
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
feature: tab stops #261
Conversation
708b83d
to
f0ad319
Compare
844f3ea
to
a4a3361
Compare
Ok, this commit is merged into python-openxml/feature/tabstops. |
e375ccc
to
ddfb2c3
Compare
Steve, your last comments were made on a temporary branch which I have now deleted. I have updated this branch based on your feedback and it now contains just the first set of acceptance tests for your review. |
b919cc8
to
7e10296
Compare
features/txt-parfmt-props.feature
Outdated
| value | | ||
| no | | ||
| some | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is still a lot of acceptance tests to stack up before any implementation, so I'm going to push through this first one just so we're not crossing objects. Then the rest should be okay in a single commit.
- This should be a Scenario: not a Scenario Outline:, because there's only one case ...
- The title should be 'Get tab stops'. object is implied in the title for all these and just adds noise to spell it out.
- the given is "Given a paragraph format", period. All paragraph format objects have a TabStops object, whether or not it contains any tab stops. And, we always start with the parent object as the given, not further up the hierarchy.
- the then clause is "Then paragraph_format.tab_stops is a TabStops object". CT_TabStops is part of the oxml/xmlchemy layer, not an API object.
- the examples go away because it's a scenario rather than a scenario outline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this acceptance test is merged in feature/tabstops on the main repo. You should rebase onto that and you can proceed with the implementation of this bit. You can submit all those commits (there might only be one), I expect it will be close enough for me to merge with any tweaks.
I'll provide feedback on the rest now, and you can submit them in commits following the ParagraphFormat.tab_stops commit(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In features/txt-parfmt-props.feature
#261 (comment):@@ -4,6 +4,17 @@ Feature: Get or set paragraph formatting properties
I need a ParagraphFormat object with read/write formatting properties
- @wip
- Scenario Outline: Get tab stops object
- Given a paragraph having tab_stop elements
Then paragraph_format.tab_stops is a CT_TabStops object
- Examples: values for paragraph_format.tab_stop elements
| value |
| no |
| some |
Okay, this acceptance test is merged in feature/tabstops on the main
repo. You should rebase onto that and you can proceed with the
implementation of this bit. You can submit all those commits (there
might only be one), I expect it will be close enough for me to merge
with any tweaks.I'll provide feedback on the rest now, and you can submit them in
commits following the ParagraphFormat.tab_stops commit(s).
Hi Steve,
I'm back from vacation and ready to dive in, but I'm confused by what
you mean here by "the implementation of this bit."
I thought the process was to get a fairly comprehensive set of
acceptance tests in place, then do a series of unit test -
implementation pairings. Am I supposed to do acceptance test - unit
test - implementation of each step as we go? If not, I'm not clear what
is needed under "implementation" for this first acceptance test. It
looks like it fails at the right spot pre-implementation to me, so I
must be missing something.
David
David K. Woods, Ph.D.
Researcher, Lead Transana Developer
Wisconsin Center for Education Research
University of Wisconsin, Madison
http://www.transana.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David, what I meant by "the implementation of this bit" was the unit-test/implementation pairing(s) that when complete allow the acceptance test I merged to pass.
Then the rest of these acceptance tests can probably come as a group; I'm assuming it compresses to four or so rather than the original nine.
It's a judgement call how many acceptance tests ahead you develop. There's certainly nothing wrong with doing them onesy-twosy at a time with implementation in-between. The problem with stacking up too many is you're getting ahead of your understanding of the implementation and raising the risk you need to go back and change something based on what you learn.
But in this case, four or so is okay, once the actual access of a TabStops object from ParagraphFormat is separated out. These would all be tests of the collection behaviors of TabStops, so it's a coherent set.
Let me know if you need more.
aba9454
to
851bfc1
Compare
@DKWoods - I rewrote the You'll see it's an entirely different approach:
Let me know if you need more. The mocks don't show up as much once we get to the next level because most of the properties on an object use actual XML (via cxml) to see that everything works all the way through the oxml layer. |
merged. |
bb2746b
to
d3bbe94
Compare
Okay, these are merged here: I added an additional test on Want to take a crack at the elaborated TabStop.position setter that does a delete+insert_in_order? I think once we have that we're pretty much ready to call this one done. |
Hi Steve,
I've checked in code that allows us to change Tab_Stop.position while
maintaining position order in TabStops.
I had to alter some existing acceptance and unit tests that assumed a
persistent TabStop reference, as the position change using the
add-and-delete method broke them. Modernizing the tests seemed like a
better approach than trying to find a hack that accomplished the goal
without invalidating the existing tests.
David
|
yep, absolutely, that's the right choice, nothing sacred about tests (unlike API) :) I should have time to merge it in this evening. Have you had a chance to try it out with any of your typical use cases, now that it's done, and see how well it suits actual usage? |
Hi Steve,
I now have this version integrated into my file parser test tool,
although not the full Transana application. My use case is a little bit
limited, in that my system only supports left-aligned tabs and does not
support leader characters at all. But in my tests, tabs work perfectly
for import and export between Word and my wxPython wxRichTextCtrl-based
editor.
David
|
Sounds great David, thanks :) I'm sure we'll hear about it soon enough if someone has a use case we haven't thought of :) |
Okay, the update TabStop.position is blended into the commit stream here: I tweaked the approach in places so might be worth checking out. Primarily here: .. but also in the acceptance test; I just combined it into one. I'll just get the API documentation wired up here and I think we should be ready to merge this one into develop. I think I'll announce it to folks to see if we can get any beta testing before it goes into the release. |
Analysis document for tab stop definition feature