[GitHub] [calcite-avatica-go] josiahg removed a comment on pull request #54: [CALCITE-4174] avatica-go should handle complex/long URLs (josiahg)

[GitBox <gi...@apache.org>]

josiahg removed a comment on pull request #54:
URL: https://github.com/apache/calcite-avatica-go/pull/54#issuecomment-673753441


   Agree that while there is some subtlety it is consistent.
   
   I’m happy to take a shot at FormatDSN().
   
   Since these are separate - though related - things, I’ll probably start a
   new ticket and branch for that effort.
   
   Agree? Are you happy with the solution and tests in this PR?
   
   On Thu, Aug 13, 2020 at 18:56 Francis Chuang <no...@github.com>
   wrote:
   
   >
   >
   > *@F21* commented on this pull request.
   >
   >
   >
   >
   > ------------------------------
   >
   >
   >
   >
   > In dsn.go
   > <https://github.com/apache/calcite-avatica-go/pull/54#discussion_r470292931>
   > :
   >
   >
   > > +		s := strings.Split(parsed.Path, "/")
   >
   > +		conf.schema = s[len(s)-1]
   >
   >
   >
   > @joshelser <https://github.com/joshelser> Good catch! Yes, that is pretty
   > subtle and could be difficult to debug. I think there should not be a
   > trailing slash after the schema (if provided) as this is the convention
   > used by a few drivers I checked. This is the case for the MySQL driver
   > you've linked as well as the postgres driver:
   > https://pkg.go.dev/github.com/lib/pq?tab=doc
   >
   >
   > In other words, https://some.host/proxied/avatica means the default
   > schema is avatica and https://some.host/proxied/avatica/ means no default
   > schema. As you mentioned, this is definitely a subtle difference, but at
   > least we are following convention, so there's consistency across drivers.
   >
   >
   > I am not opposed to moving the default schema to a query string, but it
   > seems to be a break from the standard convention and I don't think it's
   > really necessary. In addition, that would be a breaking change as well, so
   > if we do go down this path, we'd have to consider if we're breaking
   > anything else.
   >
   >
   > I think the FormatDSN() method is a good idea. We can change the
   > properties in the Config
   > <https://godoc.org/github.com/apache/calcite-avatica-go#Config> struct to
   > public (breaking change, but additive) and add a FormatDSN method to turn
   > the config back to the DSN.
   >
   >
   > The above probably sounds a bit rambly, but I personally prefer the
   > following approach:
   >
   >
   >
   >
   >
   >    - https://some.host/proxied/avatica means the schema is avatica and
   >    https://some.host/proxied/avatica/ means no schema.
   >
   >    - Let's make the properties in Config public and add a FormatDSN()
   >    method to it.
   >
   >
   >
   >
   >
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/calcite-avatica-go/pull/54#discussion_r470292931>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/ABFY3D2AQ2Y6NMTK6MRKBM3SARVQNANCNFSM4P44PQFA>
   > .
   >
   >
   > --
   
   - j
   
   Josiah Goodson
   *Senior Solutions Engineer*
   (919) 812 9358
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org