You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
I think I misunderstood what you were asking for when you said "When I am ready", to be honest, it's probably a good thing that you jumped in to finish it, as I did not have much free time.
Had a look through the code, and my only comment is I still think the user should be able to specify column width, only because from experience when dealing with formats like this, the receiving systems are fussy about the exact column widths.
Also having the ability to left or right-align columns is useful, thinking about it thought , it's usually numeric columns that are right-aligned so this could be a simple flag Right alignNumericFields
Just my thought, I know you want to keep it simple, but I think those couple of tweaks will make it useful without overcomplicating it.
The reason will be displayed to describe this comment to others. Learn more.
I probably agree with you. My initial part was i just wanted to get your original idea in and because its just additional optional options we can add both Spacing and Alignment in a separate pr. I just had difficulty wrapping my head around things and just needed the base simplified and then we can build upon it.
Let me get through the template part as well and then we can recap
On Mon, 20 Nov 2023, 20:20 Brian Voelker, ***@***.***> wrote:
I probably agree with you. My initial part was i just wanted to get your
original idea in and because its just additional optional options we can
add both Spacing and Alignment in a separate pr. I just had difficulty
wrapping my head around things and just needed the base simplified and then
we can build upon it.
Let me get through the template part as well and then we can recap
—
Reply to this email directly, view it on GitHub
<f548b6c#commitcomment-133084905>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABDIAXN2QONFNT65FWEHE5DYFO3PZAVCNFSM6AAAAAA7TO3IGGVHI2DSMVQWIX3LMV43OQ3PNVWWS5CDN5WW2ZLOOQ5TCMZTGA4DIOJQGU>
.
You are receiving this because you commented.Message ID:
<brianvoe/gofakeit/commit/f548b6c78c96880f96d2dda0b48a21e8e10c0cd2/133084905
@github.com>
f548b6c
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.
I think I misunderstood what you were asking for when you said "When I am ready", to be honest, it's probably a good thing that you jumped in to finish it, as I did not have much free time.
Had a look through the code, and my only comment is I still think the user should be able to specify column width, only because from experience when dealing with formats like this, the receiving systems are fussy about the exact column widths.
Also having the ability to left or right-align columns is useful, thinking about it thought , it's usually numeric columns that are right-aligned so this could be a simple flag Right alignNumericFields
Just my thought, I know you want to keep it simple, but I think those couple of tweaks will make it useful without overcomplicating it.
apart from that, it's looks good.
Thanks for jumping in and tweaking.
f548b6c
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.
I probably agree with you. My initial part was i just wanted to get your original idea in and because its just additional optional options we can add both Spacing and Alignment in a separate pr. I just had difficulty wrapping my head around things and just needed the base simplified and then we can build upon it.
Let me get through the template part as well and then we can recap
f548b6c
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.