Skip to content

Implement textNumberPattern 'V' virtual decimal point.#900

Merged
mbeckerle merged 1 commit intoapache:mainfrom
mbeckerle:daf-853-vp
Jan 3, 2023
Merged

Implement textNumberPattern 'V' virtual decimal point.#900
mbeckerle merged 1 commit intoapache:mainfrom
mbeckerle:daf-853-vp

Conversation

@mbeckerle
Copy link
Contributor

@mbeckerle mbeckerle commented Dec 23, 2022 •
edited
Loading

Implements 'V' a major missing feature for Cobol data.

I have also updated the DFDLSchemas Cobol example to use a snapshot of this. And that works.
See DFDLSchemas/Cobol#2

DAFFODIL-853

mbeckerle
mbeckerle commented Dec 23, 2022
View reviewed changes
@mbeckerle mbeckerle changed the title Draft: Work in progress - Implement textNumberPattern 'V' virtual decimal point. Implement textNumberPattern 'V' virtual decimal point. Dec 23, 2022
@mbeckerle
Copy link
Contributor Author

mbeckerle commented Dec 23, 2022

Ready for review now.
Works for textNumberRep standard and zoned. Has tests for both.

mbeckerle
mbeckerle commented Dec 23, 2022
View reviewed changes
@tuxji
Copy link
Contributor

tuxji commented Dec 29, 2022

DAFFODIL-853 says both V and P symbols need support, but this pull request adds support for only V. Are you planning to make DAFFODIL-853 just about V and create a new issue for P, or what?

tuxji
tuxji approved these changes Dec 29, 2022
View reviewed changes
Copy link
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

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

+1

I asked some questions and suggested a few minor changes, but the PR looks good overall.

jadams-tresys
jadams-tresys approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@jadams-tresys jadams-tresys left a comment

Choose a reason for hiding this comment

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

+1

Looks good!

@mbeckerle mbeckerle merged commit a2b5f56 into apache:main Jan 3, 2023
@mbeckerle mbeckerle deleted the daf-853-vp branch January 3, 2023 15:22
stevedlawrence
stevedlawrence reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

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

+1

This has already been merge, but I wanted to review it any way. Suggest any changes are included in changes needed when P support is added.

mbeckerle
mbeckerle commented Jan 12, 2023
View reviewed changes
Copy link
Contributor Author

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

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

@stevedlawrence Your additional review items were addressed in PR
#910
Which also adds the 'P' character feature (similar to 'V')

@mbeckerle
Copy link
Contributor Author

mbeckerle commented Jan 12, 2023

Actually I blew it creating PR 910.

The PR with the additional fixes per this PR review is
#911

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

Reviewers

@stevedlawrence stevedlawrence stevedlawrence left review comments

@tuxji tuxji tuxji approved these changes

@jadams-tresys jadams-tresys jadams-tresys approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mbeckerle @tuxji @stevedlawrence @jadams-tresys