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

Add string alignment handling to FormatWith functionality #35

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

TheodoreBrinkman
Copy link

Additional values & tests for a left-padding format string.

This type of format string does not currently work in FormatWith.
I suspect a format-string parse issue, where it assumes the first format-string delimiter is ':', rather than a potential ','.
I've been trying to find workarounds, but haven't managed to do so yet.

Added test values for left-pad formatting.
Added test for left-pad formatting.
@TheodoreBrinkman
Copy link
Author

TheodoreBrinkman commented Aug 22, 2024

FormatWith/Internal/FormatHelpers.cs around line 48/138, the parser checks for the first instance of ':', but it is possible for the overall format string to begin with ',' followed by a number indicating alignment then ':' and the rest of the format string. the ',' plus number denotes padding the value with spaces.
A positive value indicates left-padding.
A negative value indicates right-padding.

It might be worth switching to a RegEx parser for the format string, but I'm not sure how complex that would be.
Otherwise, the fix would involve finding the ':' if it's there, then looking for a preceeding ',' to parse out the number, and making sure you build the format value (defined on line 47/137) to include the ',alignment:formatString' combination as necessary.

Note: Line references appear to be 40/39, and 109/108 respectively, in the 4.0 branch.

@TheodoreBrinkman TheodoreBrinkman changed the title Patch 1 Add string alignment handling to FormatWith functionality Aug 22, 2024
@TheodoreBrinkman
Copy link
Author

TheodoreBrinkman commented Aug 23, 2024

Here's a .NET Fiddle example of the format string with alignment in action.

.Net Fiddle - Format String with Alignment

Initial swag at parsing and using both the alignment and format segments.  I don't understand the tokenization, and suspect a new handler might be needed.
Fixed some minor typos.
@TheodoreBrinkman
Copy link
Author

TheodoreBrinkman commented Aug 26, 2024

A quick .NET Fiddle sample comparing a regex parse to a IndexOf/Substring parse.
https://dotnetfiddle.net/tWPTyJ

@TheodoreBrinkman
Copy link
Author

TheodoreBrinkman commented Aug 27, 2024

@crozone I'm not familiar with the innards of the library. Can you let me know where else I might need to make changes to handle the alignment portion of a format string? I'm thinking it might be useful to provide the parsing functionality as a function that returns an object so it can be used by handlers, but I'm not certain.

Updated null/blank check on format string to include the full alignment+format string.
@TheodoreBrinkman
Copy link
Author

TheodoreBrinkman commented Aug 27, 2024

Also, if you're willing to have a breaking change for 4.0, you could re-structure the special handler designation such that it can have alignment, handler, and formatting, and provide a way to register multiple handlers, rather than requiring library users to write a single method to handle any and all special handlers they want to use.

For example: {token[,alignment][;handler][:formatString]}
{someDate,15;uppercase:yyyy MMM dd} to convert a date to 1900 JAN 24, and left-pad it out to 15 characters.

Looking at the .NET composite formatting page, this would be compatible with the existing {index[,alignment][:formatString]} notation.

Finally had a chance to work in an actual dev environment.  Sorry about the mish-mash of prior edits in this branch.
Finally had a chance to work in an actual dev environment.  Sorry about the mish-mash of prior edits in this branch.
Finally had a chance to work in an actual dev environment.  Sorry about the mish-mash of prior edits in this branch.
Finally had a chance to work in an actual dev environment.  Sorry about the mish-mash of prior edits in this branch.

Moved new tests into their own file.
Finally had a chance to work in an actual dev environment.  Sorry about the mish-mash of prior edits in this branch.

Moved tests into their own file.
Additional tests to validate FormatWith usage against the new code.
Copy link
Author

@TheodoreBrinkman TheodoreBrinkman left a comment

Choose a reason for hiding this comment

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

Passes all existing tests and newly created tests.

@TheodoreBrinkman
Copy link
Author

I've also got a set of changes for the 4.0 branch, but I've always been in TFS shops, so I'm having issues figuring out the process for Git in Visual Studio. And it'll probably need a look at whether I've missed anything regarding the use of ReadOnlySpan rather than String.

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.

1 participant