Skip to content
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

Added ability to run sql queries on sqlite files #294

Merged
merged 1 commit into from
Mar 3, 2020

Conversation

LesykMelnychuk
Copy link

Added ability to run execution and selection sql queries during sqlite db exploration.

@NSExceptional NSExceptional self-assigned this Jul 25, 2019
@NSExceptional
Copy link
Collaborator

There's a lot of changes here. so when I have time, I'm going to review this on my machine and clean up the commit history a bit. I do want to merge this eventually though!

@LesykMelnychuk
Copy link
Author

Great

@NSExceptional
Copy link
Collaborator

NSExceptional commented Jul 30, 2019

Hmmm, it appears you didn't check the option to give maintainers access to your fork when you created it. Could you give me commit access to your fork?

At least, that's the error I'm getting:

 ! [remote rejected] LesykMelnychuk/execute_sql -> feature/execute_sql (permission denied)
error: failed to push some refs to 'https://[email protected]/LesykMelnychuk/FLEX.git'

Edit: I have access to master only, didn't know that. Never mind!

@NSExceptional
Copy link
Collaborator

NSExceptional commented Jul 30, 2019

Okay, I got everything squashed into a single commit on master and properly rebased.

After looking over your code, it doesn't match our coding conventions. Can you go through each of the files you've added or changed and make sure you're following the style set out in this codebase? Some examples:

  • Do not indent @property declarations (I took care of this already)
  • Never put spaces between arguments and their labels
    • [self foo: bar];
    • [self foo:bar];
  • Never put spaces after argument labels in method declarations
    • - (void)foo: (id)foo and: (NSString **)error
    • - (void)foo:(id)foo error:(NSString **)error
  • Pointer markers go with the variable name, not the type
    • UILabel* textLabel;
    • UILabel *textLabel;
  • There is no need to manually @synthesize properties unless you override the getter and the setter (so, use self.isSelectionType instead of isSelectionType)
  • Prefer properties over ivars in almost all cases
  • Use dot syntax for accessing properties
    • [str UTF8String]
    • str.UTF8String
    • [self notAProperty];

Also, what's SQLITE_HAS_CODEC for? Can we get rid of that preprocessor check?

@LesykMelnychuk
Copy link
Author

Ok - I'll fix code style

return error;
}

- (NSArray<NSDictionary<NSString *, id> *> *)executeSelectionQuery: (NSString *)sql and: (NSString **)error {
Copy link
Collaborator

@NSExceptional NSExceptional Aug 6, 2019

Choose a reason for hiding this comment

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

Suggested change
- (NSArray<NSDictionary<NSString *, id> *> *)executeSelectionQuery: (NSString *)sql and: (NSString **)error {
- (NSArray<NSDictionary<NSString *, id> *> *)executeSelectionQuery:(NSString *)sql and:(NSString **)error {

Make sure you correct the spacing of method declarations

}
return self;
}

- (void)viewWillAppear: (BOOL)animated {
[super viewWillAppear: animated];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
[super viewWillAppear: animated];
[super viewWillAppear:animated];


[alertController addAction: executeAction];
[alertController addAction: selectAction];
[alertController addAction: cancelAction];
Copy link
Collaborator

@NSExceptional NSExceptional Aug 6, 2019

Choose a reason for hiding this comment

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

Suggested change
[alertController addAction: cancelAction];
[alertController addAction:cancelAction];

Spacing on all three of these lines


- (void)submitPressed
{
NSString *text = self.textView.text == nil ? @"" : self.textView.text;
Copy link
Collaborator

@NSExceptional NSExceptional Aug 6, 2019

Choose a reason for hiding this comment

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

Suggested change
NSString *text = self.textView.text == nil ? @"" : self.textView.text;
NSString *text = self.textView.text ?: @"";

Shorthand ternary syntax is preferred for clarity; it's similar to Swift's ?? nil coalescing operator

NSMutableDictionary<NSString *, id> *dict = [NSMutableDictionary dictionaryWithCapacity:num_cols];

int columnCount = sqlite3_column_count(pstmt);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid excess whitespace, group lines together logically

actionWithTitle: @"Execute SQL"
style: UIAlertActionStyleDefault
handler: ^(UIAlertAction *action) {
FLEXSQLCommandExecutionViewController* controller = [FLEXSQLCommandExecutionViewController new];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
FLEXSQLCommandExecutionViewController* controller = [FLEXSQLCommandExecutionViewController new];
FLEXSQLCommandExecutionViewController *controller = [FLEXSQLCommandExecutionViewController new];

There are still a lot of these

NSString *columnName = [NSString stringWithUTF8String:sqlite3_column_name(pstmt, columnIdx)];
id objectValue = [self objectForColumnIndex:columnIdx stmt:pstmt];

[dict setObject:objectValue forKey:columnName];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
[dict setObject:objectValue forKey:columnName];
dict[columnName] = objectValue;

Use subscripting syntax wherever possible

@NSExceptional
Copy link
Collaborator

I left some comments, there's a lot you missed 😅 I didn't mark them all, just a bunch I found.

Also, before you go any further, make sure your HEAD is at origin/master to avoid the merge commit. You don't need to merge, just fast forward. Lemme know if you need help with avoiding a merge commit.

@LesykMelnychuk
Copy link
Author

I left some comments, there's a lot you missed 😅 I didn't mark them all, just a bunch I found.

Also, before you go any further, make sure your HEAD is at origin/master to avoid the merge commit. You don't need to merge, just fast forward. Lemme know if you need help with avoiding a merge commit.

Fixed comments you made - fixed, were could found.
Thanks, that you are interested in my pull request!

@NSExceptional
Copy link
Collaborator

@revolter Can you tell me what this macro and code segment is for? 😅I'm trying to decipher some of this old code, I've never used the sqlite APIs

https://github.com/Flipboard/FLEX/blame/0de3a9d65c54533848b6721d0d66bc3fb0d740fa/Classes/GlobalStateExplorers/DatabaseBrowser/FLEXSQLiteDatabaseManager.m#L38

@revolter
Copy link
Contributor

revolter commented Aug 7, 2019

That macro is set by the SQLCipher pod and the code is used to decrypt a crypted database after opening it, or else any query would fail.

@NSExceptional
Copy link
Collaborator

Thanks! I'm trying to add comments to these files

@NSExceptional
Copy link
Collaborator

@LesykMelnychuk Can I ask why you added executeNonSelectQuery: and executeSelectionQuery:error: instead of making use of executeQuery:?

I've read the code more thoroughly now and there seems to be a lot of duplication now. At the very least, would you be able to make those two methods call into executeQuery:?

@LesykMelnychuk
Copy link
Author

LesykMelnychuk commented Aug 12, 2019

@NSExceptional Hi, cause executeSelectionQuery:error returns result of selection and it has difference calling functions inside

@NSExceptional
Copy link
Collaborator

Your executeSelectionQuery:error: is almost identical to the original executeQuery: unless I'm overlooking something. You return an error in an argument but that's the only difference

@LesykMelnychuk
Copy link
Author

Your executeSelectionQuery:error: is almost identical to the original executeQuery: unless I'm overlooking something. You return an error in an argument but that's the only difference

Understud, - fixed.

@LesykMelnychuk
Copy link
Author

Hello @NSExceptional , can you please merge this pull ?

@NSExceptional
Copy link
Collaborator

Hey @LesykMelnychuk,

This PR isn't in a state where I'm ready to merge it. I have a fork of your branch I started working on a while back but I got sidetracked and it fell off my todo list. I will try to get it done and merged this week. (At the moment I don't even remember what I was working on specifically, probably adding comments or something.)

@revolter
Copy link
Contributor

revolter commented Feb 6, 2020

Shouldn't I fix the conflicts in #305, modify things if needed, merge it, and then rebase this, instead of having f7659a8 here?

@NSExceptional
Copy link
Collaborator

@revolter @LesykMelnychuk I'm currently in the process of rebasing and resolving conflicts between this branch and the 4.0 branch, should have it done tomorrow sometime

@NSExceptional NSExceptional changed the base branch from master to 4.0-flex March 3, 2020 18:47
@NSExceptional NSExceptional merged commit af12273 into FLEXTool:4.0-flex Mar 3, 2020
@NSExceptional
Copy link
Collaborator

Accidentally named the branch 4.0-flex when I meant to just name it 4.0, so I renamed it just now, FYI

@revolter
Copy link
Contributor

revolter commented Mar 3, 2020

What I meant was that this PR included #305, so you merged 2 things at the same time, while #305 looks now like it wasn't merged.

@NSExceptional
Copy link
Collaborator

Are you saying @LesykMelnychuk somehow included #305 in this original PR, and that I unknowingly discarded it when I made my changes and merged this just now?

@revolter
Copy link
Contributor

revolter commented Mar 3, 2020

Oooh, it was probably incorrectly rebased. Now it's ok, sorry I didn't check again, today. So it's all good 👍

@NSExceptional
Copy link
Collaborator

I'm so confused :P What's wrong?

@revolter
Copy link
Contributor

revolter commented Mar 3, 2020

I am what's wrong 😆 Sorry for the confusion, and completely disregard this conversation. As I said, everything is ok 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants