Skip to content

Justin#34

Merged
jhs507 merged 8 commits intomasterfrom
justin
May 5, 2021
Merged

Justin#34
jhs507 merged 8 commits intomasterfrom
justin

Conversation

@jhs507
Copy link
Collaborator

@jhs507 jhs507 commented May 5, 2021

CRHMcode gcc now has a system for processing option flags. These flags have implemented options to specify output file format and time stamp format for the resulting output.

@jhs507 jhs507 linked an issue May 5, 2021 that may be closed by this pull request
@KevinShook
Copy link
Collaborator

Sorry to leave this so late to comment (was busy with other stuff), but why are we outputting MMDDYYYY as well as YYYYMMDD?
This makes the dates extremely confusing to parse, both for humans and for computers.
Who needs MMDDYYYY?

@jhs507
Copy link
Collaborator Author

jhs507 commented May 5, 2021

I have only included MMDDYYYY as an option because there is code in the core of crhm that specifies it as a possible output. I do not think it is a good format but I wanted to avoid the possibility of accidentally removing functionality that someone uses.

@KevinShook
Copy link
Collaborator

The problem for me is that it's not currently used and I will have to write code to parse it.

@jhs507
Copy link
Collaborator Author

jhs507 commented May 5, 2021

I can remove it if no one needs it. But you shouldn't have to parse it if no one is actually using it.

@KevinShook
Copy link
Collaborator

As soon as someone does, it will break my R package. I guess I can just allow that to happen, and can have a message that it's their funeral if they use such a dumb format.

@jhs507
Copy link
Collaborator Author

jhs507 commented May 5, 2021

I see.

Well I think it might be in the best interest to remove the MMDDYYYY format as an option then. It restoring it would be trivial but I agree that in its current state there really seems to only be downside to keeping it.

@loganxingfang
Copy link
Collaborator

I see.

Well I think it might be in the best interest to remove the MMDDYYYY format as an option then. It restoring it would be trivial but I agree that in its current state there really seems to only be downside to keeping it.

No, to keep thing easy, this option is not needed.

@KevinShook
Copy link
Collaborator

Thanks very much. I know it's a pain, and it seems like a small thing, but writing downstream code, I need to
be able to tell what format the date/time it, since it's not documented in the file.
At least ISO has the "T" separator, and Excel date/times have periods, so I can discriminate between them

@jhs507 jhs507 merged commit f56bda0 into master May 5, 2021
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.

Output file of the Borland version

3 participants