Skip to content

Conversation

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2021

This pull request fixes 1 alert when merging b3f3f61 into b1ac06f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2021

This pull request fixes 1 alert when merging b19177b into b1ac06f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Dec 16, 2021

This pull request fixes 1 alert when merging 2520ed4 into b1ac06f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Dec 16, 2021

This pull request fixes 1 alert when merging c694294 into b1ac06f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Dec 21, 2021

This pull request fixes 1 alert when merging 1ac624a into b1ac06f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@Rangababu-R Rangababu-R requested a review from alakendu January 6, 2022 08:07
@lgtm-com
Copy link

lgtm-com bot commented Jan 6, 2022

This pull request fixes 1 alert when merging 50d2f0c into b1ac06f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2022

This pull request fixes 1 alert when merging e8a28dc into 9c30f74 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@Rangababu-R Rangababu-R force-pushed the py_go_diff branch 2 times, most recently from b2e7da7 to 84478f1 Compare January 10, 2022 10:47
@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2022

This pull request fixes 1 alert when merging ae969d0 into c8ceea7 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2022

This pull request fixes 1 alert when merging 5621dfa into c8ceea7 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2022

This pull request fixes 1 alert when merging 1c50076 into c8ceea7 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2022

This pull request fixes 1 alert when merging 70f070a into c8ceea7 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2022

This pull request fixes 1 alert when merging 5d0c4a5 into c8ceea7 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

Copy link
Contributor

@ajbalogh ajbalogh left a comment

Choose a reason for hiding this comment

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

LGTM

@ashutshkumr
Copy link
Contributor

ashutshkumr commented Jan 21, 2022

I would suggest some error message changes in order to ensure that both python and go refer to same names in the data models. This should be equivalent for error messages returned from both Python and Go.

  1. Do not expose internal names in error message:

    PrefixConfig.required_object is a required field from path prefixConfig.Validate()
    

    Recommended:

    required field `prefix_config.required_object` must not be empty
    
  2. Rephrasing error message for length

    length of PrefixConfig.str_len shall be in the range of [3, 6] but Got 2 from path prefixConfig.Validate()
    

    Recommended:

    length of field `prefix_config.str_len` must be in range [3, 6], instead of `2`
    
  3. For lists, I could not reproduce the error message due to another issue mentioned here. However I noticed we're mentioning Items() in the error message, which we shouldn't.

    required field `prefix_config.j[0].j_a.e_b` must not be empty
    
  4. Incorrect format errors

    invalid hex addresses at indices 2 on PrefixConfig.hex_slice from path prefixConfig.Validate()
    invalid ipv4 address 1.1.1 on LObject.ipv4 from path prefixConfig.Validate().L
    

    Recommended:

    value of `prefix_config.hex_slice[2]` must be a valid hexadecimal string, instead of `jkl`
    value of `prefix_config.l.ipv4` must be a valid ipv4 address string, instead of `1.1.1`
    

Will add more points as I find them (w.r.t. error messages)

@ashutshkumr
Copy link
Contributor

ashutshkumr commented Jan 21, 2022

Following snippet runs without issues, even though I did not set EB (required field) in ja.

func TestNewAndSet(t *testing.T) {
	c := openapiart.NewPrefixConfig()
	c.RequiredObject()
	c.SetA("hello")
	ja := c.J().Add().JA()
	ja.SetEA(2.4)
	c.SetE(openapiart.NewEObject().SetEA(123.456).SetEB(453.123))
	c.SetF(openapiart.NewFObject().SetFA("fa string"))
	yaml1, err := c.E().ToYaml()
	assert.Nil(t, err)
	yaml2, err := c.F().ToYaml()
	assert.Nil(t, err)
	log.Println(yaml1)
	log.Println(yaml2)
	log.Println(c.ToYaml())
}

This is a known issue btw (probably because required fields are of primitive types). Mentioned it here just for reference.

@ashutshkumr
Copy link
Contributor

For Clone, try converting to and from PbText instead of JSON since former would be faster (since our core data structure is in protobuf and not JSON)

@lgtm-com
Copy link

lgtm-com bot commented Feb 14, 2022

This pull request introduces 1 alert when merging 4dc0dd0 into 3249f32 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Feb 25, 2022

This pull request introduces 1 alert when merging 5fd26d6 into 0ec7bab - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Apr 4, 2022

This pull request introduces 1 alert when merging b2225fd into 261ef08 - view on LGTM.com

new alerts:

  • 1 for Unused import

@ashutshkumr
Copy link
Contributor

shall be merged as part of #320

@Vibaswan Vibaswan deleted the py_go_diff branch June 18, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants