feat: add UUID, date, and datetime helpers for terraform usage #96

rmkeezer · 2021-02-22T15:51:19Z

Add helper methods for casting strfmt UUID, Date, and DateTime literals to pointers needed by the go setter methods. Also, this is only parsing dates with RFC3339. This likely needs to be updated in the future to allow other formats to be used.

This is used in the sdk generator PR #873

CLAassistant · 2021-02-22T15:51:23Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ padamstx
❌ rmkeezer
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mkistler

I think the date/time conversion methods should have error returns and use the date-time conversion support we have already built into the core.

mkistler · 2021-02-23T14:07:15Z

v5/core/utils.go

+
+// ParseDateTime parses and returns a strfmt DateTime pointer
+func ParseDateTime(dateString string) *strfmt.DateTime {
+	formattedTime, _ := time.Parse(time.RFC3339, dateString)


You should use strfmt.ParseDateTime here. It has already been set up to handle all the date-time formats we need.

Updated to use strfmt.ParseDateTime with error handling!

mkistler · 2021-02-23T14:34:07Z

v5/core/utils.go

@@ -182,6 +188,20 @@ func GetCurrentTime() int64 {
 	return time.Now().Unix()
 }

+// ParseDate parses and returns a strfmt Date pointer
+func ParseDate(dateString string) *strfmt.Date {


It seems like we should have an error return on this method.

Updated with an error return! The generator PR was also update to catch this error return and use a variable name that wont run into redeclaration issues, new change here: https://github.ibm.com/CloudEngineering/openapi-sdkgen/pull/873/files#diff-9f83f4d9d6cba74e36a0c6fd148b83e9R936

mkistler · 2021-02-23T14:34:30Z

v5/core/utils.go

+}
+
+// ParseDateTime parses and returns a strfmt DateTime pointer
+func ParseDateTime(dateString string) *strfmt.DateTime {


I think we should have an error return on this method.

padamstx

Looks good so far. Just had some minor comments. Also, I think it would be good to add at least one negative test for ParseDate() and ParseDateTime().

padamstx · 2021-03-02T00:37:22Z

v5/core/utils_test.go

+	dateVar := strfmt.Date(time.Now())
+	fmtDate, err := ParseDate(dateVar.String())
+	assert.Nil(t, err)
+	assert.True(t, dateVar.String() == fmtDate.String())


Suggested change

assert.True(t, dateVar.String() == fmtDate.String())

assert.Equal(t, dateVar.String(), fmtDate.String())

padamstx · 2021-03-02T00:37:59Z

v5/core/utils_test.go

+	var fmtDTime *strfmt.DateTime
+	fmtDTime, err = ParseDateTime(dateTimeVar.String())
+	assert.Nil(t, err)
+	assert.True(t, dateTimeVar.String() == fmtDTime.String())


Suggested change

assert.True(t, dateTimeVar.String() == fmtDTime.String())

assert.Equal(t, dateTimeVar.String(), fmtDTime.String())

padamstx · 2021-03-02T00:42:24Z

v5/core/utils.go

@@ -182,6 +188,25 @@ func GetCurrentTime() int64 {
 	return time.Now().Unix()
 }

+// ParseDate parses and returns a strfmt Date pointer


Suggested change

// ParseDate parses and returns a strfmt Date pointer

// ParseDate parses the specified RFC3339 full-date string (YYYY-MM-DD) and returns a strfmt.Date instance.

padamstx · 2021-03-02T00:43:46Z

v5/core/utils.go

+	return &formattedDate, nil
+}
+
+// ParseDateTime parses and returns a strfmt DateTime pointer


Suggested change

// ParseDateTime parses and returns a strfmt DateTime pointer

// ParseDateTime parses the specified date-time string and returns a strfmt.DateTime instance.

padamstx

LGTM

mkistler

This looks good but I'd like a small change in the interface.

mkistler · 2021-03-02T19:33:33Z

v5/core/utils.go

+}
+
+// ParseDateTime parses the specified date-time string and returns a strfmt.DateTime instance.
+func ParseDateTime(dateString string) (fmtDTime *strfmt.DateTime, err error) {


I think it would be better for this method to return a strfmt.DateTime rather than a pointer to strfmt.DateTime. This is perhaps a question of style, and thus somewhat subjective, but in my defense I will note that strfmt.ParseDateTime returns a strfmt.DateTime.

mkistler · 2021-03-02T19:34:26Z

v5/core/utils.go

@@ -182,6 +188,25 @@ func GetCurrentTime() int64 {
 	return time.Now().Unix()
 }

+// ParseDate parses the specified RFC3339 full-date string (YYYY-MM-DD) and returns a strfmt.Date instance.
+func ParseDate(dateString string) (fmtDate *strfmt.Date, err error) {


Here also, I think we should return a strfmt.Date rather than a pointer.

mkistler

Looks good! 👍

# [5.1.0](v5.0.3...v5.1.0) (2021-03-04) ### Features * add UUID, date, and datetime helpers for terraform usage ([#96](#96)) ([e651369](e651369))

ibm-devx-automation · 2021-03-04T20:02:34Z

🎉 This PR is included in version 5.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

rmkeezer requested review from mkistler and padamstx February 22, 2021 15:51

rmkeezer force-pushed the add_uuid_date_helper branch 2 times, most recently from 975aaa1 to 1e6c007 Compare February 22, 2021 17:19

mkistler suggested changes Feb 23, 2021

View reviewed changes

rmkeezer force-pushed the add_uuid_date_helper branch from 1e6c007 to 936e78b Compare February 23, 2021 14:37

rmkeezer requested a review from mkistler February 23, 2021 15:09

padamstx requested changes Mar 2, 2021

View reviewed changes

rmkeezer force-pushed the add_uuid_date_helper branch 3 times, most recently from a47eb03 to c805b87 Compare March 2, 2021 11:15

rmkeezer requested a review from padamstx March 2, 2021 15:26

padamstx approved these changes Mar 2, 2021

View reviewed changes

mkistler suggested changes Mar 2, 2021

View reviewed changes

rmkeezer force-pushed the add_uuid_date_helper branch from c805b87 to 271a2ea Compare March 3, 2021 11:34

feat: add UUID, date, and datetime helpers for terraform usage

7f4477e

rmkeezer force-pushed the add_uuid_date_helper branch from 271a2ea to 7f4477e Compare March 3, 2021 11:43

rmkeezer requested a review from mkistler March 3, 2021 11:50

refactor: move new date functions to datetime.go

38b777f

mkistler approved these changes Mar 4, 2021

View reviewed changes

padamstx merged commit e651369 into main Mar 4, 2021

padamstx deleted the add_uuid_date_helper branch March 4, 2021 19:56

ibm-devx-automation pushed a commit that referenced this pull request Mar 4, 2021

chore(release): 5.1.0 release notes [skip ci]

ca5d4bf

# [5.1.0](v5.0.3...v5.1.0) (2021-03-04) ### Features * add UUID, date, and datetime helpers for terraform usage ([#96](#96)) ([e651369](e651369))

ibm-devx-automation added the released label Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add UUID, date, and datetime helpers for terraform usage #96

feat: add UUID, date, and datetime helpers for terraform usage #96

rmkeezer commented Feb 22, 2021

CLAassistant commented Feb 22, 2021 •

edited

Loading

mkistler left a comment

mkistler Feb 23, 2021

rmkeezer Feb 23, 2021

mkistler Feb 23, 2021

rmkeezer Feb 23, 2021 •

edited

Loading

mkistler Feb 23, 2021

padamstx left a comment

padamstx Mar 2, 2021

padamstx Mar 2, 2021

padamstx Mar 2, 2021

padamstx Mar 2, 2021

padamstx left a comment

mkistler left a comment

mkistler Mar 2, 2021

mkistler Mar 2, 2021

mkistler left a comment

ibm-devx-automation commented Mar 4, 2021

	assert.True(t, dateVar.String() == fmtDate.String())
	assert.Equal(t, dateVar.String(), fmtDate.String())

	assert.True(t, dateTimeVar.String() == fmtDTime.String())
	assert.Equal(t, dateTimeVar.String(), fmtDTime.String())

	// ParseDate parses and returns a strfmt Date pointer
	// ParseDate parses the specified RFC3339 full-date string (YYYY-MM-DD) and returns a strfmt.Date instance.

	// ParseDateTime parses and returns a strfmt DateTime pointer
	// ParseDateTime parses the specified date-time string and returns a strfmt.DateTime instance.

feat: add UUID, date, and datetime helpers for terraform usage #96

feat: add UUID, date, and datetime helpers for terraform usage #96

Conversation

rmkeezer commented Feb 22, 2021

CLAassistant commented Feb 22, 2021 • edited Loading

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmkeezer Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

ibm-devx-automation commented Mar 4, 2021

CLAassistant commented Feb 22, 2021 •

edited

Loading

rmkeezer Feb 23, 2021 •

edited

Loading