- 
                Notifications
    
You must be signed in to change notification settings  - Fork 37
 
T360 sql editor bug #28
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.
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
base: master
Are you sure you want to change the base?
Conversation
fix problems with sql parser fix some other problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting errors while performing some queries. Need to investigate further
        
          
                lib/sql_parser.js
              
                Outdated
          
        
      | return null; | ||
| } | ||
| 
               | 
          ||
| function parseHaveingConditions(str) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in Haveing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| } | ||
| 
               | 
          ||
| function setFieldsConstructorsType(fieldsList, LocalState) { | ||
| let isConstructorsFinded = false; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isConstructorsFound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| 
               | 
          ||
| return result; | ||
| 
               | 
          ||
| function determineFieldsBySavedConstructors(fieldsArr, constructors) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, don't mutate function parameter fieldsArr http://eslint.org/docs/rules/no-param-reassign
It is better to create a clone and make manipulations on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should use map to return cloned array
| return fieldsArr; | ||
| } | ||
| 
               | 
          ||
| function determineFieldsByModel(fieldsArr) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with mutations here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| } | ||
| } | ||
| 
               | 
          ||
| function setFieldsConstructorsType(fieldsList, LocalState) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets use LocalState only inside setSQLQuery function. This setFieldsConstructorsType function should only return some result that we can use for setting to LocalState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
| return field; | ||
| }); | ||
| const determineFieldsByModel = (fieldsArr, pivotModel) => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
determineFieldsBy... functions could be defined outside of target determineDefaultFields, e.g. as plain file variables
        
          
                lib/sql_parser.js
              
                Outdated
          
        
      | return null; | ||
| 
               | 
          ||
| 
               | 
          ||
| function removeBraces(queryPart) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can also be moved outside from function.
        
          
                lib/sql_parser.js
              
                Outdated
          
        
      | 
               | 
          ||
| function getWhere(query) { | ||
| let where = query.match(/\swhere\s+([\w([\])"'`.,/=<>\s]+$)/i); | ||
| let where = query.match(/\swhere\s+([\w([\])"'`.,/=<>\s]+$)/im); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace let
        
          
                lib/sql_parser.js
              
                Outdated
          
        
      | const andOr = str.match(/\s+(and|or)\s+/gi); | ||
| 
               | 
          ||
| if (andOr) { | ||
| let rest = str.slice(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove let
        
          
                lib/sql_parser.js
              
                Outdated
          
        
      | } else { | ||
| filters.push(str); | ||
| } | ||
| return filters.map((filter, i) => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split to smaller functions
No description provided.