Skip to content

#1835: Support multiple date formats in Helper scripts #1837

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 31, 2021
Merged

#1835: Support multiple date formats in Helper scripts #1837

merged 5 commits into from
Jul 31, 2021

Conversation

@benedeki
Copy link
Collaborator

  • added configuration for date parsing type
  • .gitattributes changed to hopefully better handle CRLF in native Windows files
  • LF -> CRLF in *.cmd and *.bat files

I needed to implement the partial changes for my testing for other issues. So I thought to make it more general and include in code-base.

Release notes:
Configuration of the date parsing (locale dependent) to correctly name the log file.

@benedeki
#1835: Support multiple date formats in Helper scripts
1987301 
* added configuration for date parsing type
* .gitattributes changed to hopefully better handle CRLF in native Windows files
* LF -> CRLF in *.cmd and *.bat files
@benedeki benedeki self-assigned this Jun 30, 2021
@dk1844 dk1844 linked an issue Jun 30, 2021 that may be closed by this pull request
Support multiple date formats in Helper scripts #1835
Closed
dk1844
dk1844 previously approved these changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

LGTM (just read the code)

dk1844
dk1844 reviewed Jun 30, 2021
View reviewed changes
scripts/cmd/_run_enceladus.cmd Outdated
::SET YYYY=%date:~10,4%
::SET MM=%date:~4,2%
::SET DD=%date:~7,2%
IF %DATE_PARSING_TYPE%==parser1 (
Copy link
Contributor

@dk1844 dk1844 Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In the most usual cases, this will probably work fine.
In general, the users can change the formats, so even the position-based parsing may not be sufficient (e.g. for the situation of (non)-padding with 0, short/long year, etc.

I suggest taking inspiration at https://ss64.com/nt/syntax-gettime.html | https://ss64.com/nt/syntax-gmt.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice. I will try to implement this in a free moment. This is no high priority. I pushed the change only because I needed a different parsing, so made it switchable, albeit not the smartest. 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree that the current solution is probably good enough for the use case.

It just made sense to provide some information in the problematics in the broader sense, we may very well decide to leave it as-is, it would just be a bit more informed decision 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be universal now 🤞
Also added configuration to prevent editor conversion from CRLF to LF

@benedeki benedeki added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jul 12, 2021
@benedeki
#1835: Support multiple date formats in Helper scripts
31cbffd 
* automatic universal date & time parsing regardless of locale in Windows helper scripts
* CRLF for *.cmd and *.bat files in editor
@benedeki benedeki removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jul 13, 2021
dk1844
dk1844 approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

  • code reviewed
  • pulled
  • built
  • run and disected

LGTM.

Personal remark: Oh, I so don't want to be writing stuff for CMD (Powershell seems to fill the blanks, at least. And the GOTO command 🙄 )

scripts/cmd/_run_enceladus.cmd
Comment on lines +665 to +667
SET /A FDATE=%%F*10000+%%D*100+%%A
:: prefixing 1000000 to ensure the hours is two digit (starting with 0 before 10)
SET /A FTIME=1000000+%%B*10000+%%C*100+%%E
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very interesting approach, but why not. I would have naturally attempted to created a number-padding method, but this works well, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found it the easiest. Batch scripts are designed to manipulate strings, as I see it 😉

scripts/cmd/_run_enceladus.cmd
SET DD=%date:~8,2%
::Date&Time parsing
FOR /F "skip=1 tokens=1-6" %%A IN ('WMIC Path Win32_LocalTime Get Day^,Hour^,Minute^,Month^,Second^,Year /Format:table') DO (
if "%%B" NEQ "" (
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, naturally, I would have looked for cmd alternative for shell myCommand | head -n2 | tail -n1, but this seems to be simply available only in powershell 🙄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I admit, I found this on the Internet, not my invention at all.

@benedeki
Copy link
Collaborator Author

Personal remark: Oh, I so don't want to be writing stuff for CMD (Powershell seems to fill the blanks, at least. And the GOTO command 🙄 )

Funny I never learned PowerShell, and I find batch script surprisingly powerful. 👼
And the GOTO - seemed as the clearest solution, could have done it with reverting the if, perhaps with some {...} 🤷

@benedeki
Copy link
Collaborator Author

benedeki commented Jul 23, 2021 •
edited by dk1844
Loading 

* [x]  code reviewed
* [x]  pulled
* [x]  built
* [x]  run and disected

Based on this I dare to add PR: Testes label. 😉

// @dk1844 replying: yes, of course, I forgot to label it.

@benedeki benedeki added the PR:tested Only for PR - PR was tested by a tester (person) label Jul 23, 2021
@benedeki
Copy link
Collaborator Author

Release notes used:
Windows Helper scripts are now parse the date independently of locale (for the log file name purpose).

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@benedeki benedeki merged commit 81f75c3 into develop Jul 31, 2021
@benedeki benedeki deleted the feature/1835-support-multiple-date-formats-in-helper-scripts branch July 31, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dk1844 dk1844 dk1844 approved these changes

@AdrianOlosutean AdrianOlosutean Awaiting requested review from AdrianOlosutean

@DzMakatun DzMakatun Awaiting requested review from DzMakatun

@HuvarVer HuvarVer Awaiting requested review from HuvarVer

@lokm01 lokm01 Awaiting requested review from lokm01

Assignees

@benedeki benedeki

Labels

PR:tested Only for PR - PR was tested by a tester (person)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support multiple date formats in Helper scripts

3 participants

@benedeki @dk1844