Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-51120

'format' on CREATE ANALYTICS VIEW statement results in unnatural layout

    XMLWordPrintable

Details

    • Bug
    • Status: Reopened
    • Major
    • Resolution: Unresolved
    • 7.1.0
    • Morpheus
    • UI
    • Untriaged
    • 1
    • Unknown
    • UI 2022-Feb

    Description

      Given a CREATE ANALYTICS VIEW, where the view contents are enclosed in (), the format directive puts all of the view contents into a single line, to not break what is enclosed within the ()s. Subsequent lines are horizontally aligned with the SELECT statement after the closing of the (), further complicating the readability of the formatted statement.

      Before format:

      After format:
      ... and after horizontally scrolling the query box all the way to the right:

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          Build couchbase-server-7.1.0-2375 contains ns_server commit ab0f9ee with commit message:
          MB-51120 format CREATE ANALYTICS VIEW queries

          build-team Couchbase Build Team added a comment - Build couchbase-server-7.1.0-2375 contains ns_server commit ab0f9ee with commit message: MB-51120 format CREATE ANALYTICS VIEW queries
          umang.agrawal Umang added a comment - - edited

          Tried with build 7.1.0-2534, the formatted statement still looks unnatural. Attaching image.
          https://issues.couchbase.com/secure/attachment/180582/Screenshot%202022-03-29%20at%2012.18.07%20PM.png

          umang.agrawal Umang added a comment - - edited Tried with build 7.1.0-2534, the formatted statement still looks unnatural. Attaching image. https://issues.couchbase.com/secure/attachment/180582/Screenshot%202022-03-29%20at%2012.18.07%20PM.png

          Hi Eben Haber, it looks the code block that causes this is 

          lines: 237-241 in `ui/libs/ace/query-formatter.js`:

          else for (fs = ar[ix].length - 1; fs >= 0; fs--)
            if (ar[ix].charAt(fs) == ' ') {
              fs++;
              break;
            } 

          Removing it seems to fix it, but it feels like this will break other forms of formatting,

          would this a dangerous change?

           

          matthew.dawber Matthew Dawber added a comment - Hi Eben Haber , it looks the code block that causes this is  lines: 237-241 in `ui/libs/ace/query-formatter.js`: else for (fs = ar[ix].length - 1 ; fs >= 0 ; fs--) if (ar[ix].charAt(fs) == ' ' ) {   fs++; break ; } Removing it  seems  to fix it, but it feels like this will break other forms of formatting, would this a dangerous change?  
          eben Eben Haber added a comment -

          Michael Blow FYI, formatting of the "AS SELECT" query would be improved if the query were surrounded by parens, e.g.: "AS (SELECT .... )". Are parens permitted there in sql++?

          Matthew Dawber I have a change that works, will send to you via slack for testing.

          eben Eben Haber added a comment - Michael Blow FYI, formatting of the "AS SELECT" query would be improved if the query were surrounded by parens, e.g.: "AS (SELECT .... )". Are parens permitted there in sql++? Matthew Dawber I have a change that works, will send to you via slack for testing.
          graeme.clark Graeme Clark added a comment -

          Hi all,

          Eben Haber is currently on leave.

          Michael Blow reassigning this for UI intervention.

          Pavel Blagodov can you take a look at this one for us? Thank you!

          graeme.clark Graeme Clark added a comment - Hi all, Eben Haber is currently on leave. Michael Blow reassigning this for UI intervention. Pavel Blagodov can you take a look at this one for us? Thank you!
          eben Eben Haber added a comment -

          Changing fix version to Morpheus, this needs more time to properly fix than would be possible for 7.1.2.

          eben Eben Haber added a comment - Changing fix version to Morpheus, this needs more time to properly fix than would be possible for 7.1.2.

          People

            eben Eben Haber
            michael.blow Michael Blow
            Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

              Created:
              Updated:

              Gerrit Reviews

                There are no open Gerrit changes

                PagerDuty