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

Add --author, -m to COMMIT. Add --author to MERGE() #1090

Merged
merged 2 commits into from Dec 13, 2020

Conversation

VinaiRachakonda
Copy link
Contributor

@VinaiRachakonda VinaiRachakonda commented Dec 8, 2020

COMMIT('-m', 'hi', '--author', 'John Doe john@doe.com')
MERGE('feature-branch', '--author', 'John Doe john@doe.com')

@VinaiRachakonda VinaiRachakonda changed the title [WIP] Initial commit. Compiles but errors [WIP] Add --author, -m to COMMIT. Add --author to MERGE() Dec 9, 2020
@VinaiRachakonda VinaiRachakonda changed the title [WIP] Add --author, -m to COMMIT. Add --author to MERGE() Add --author, -m to COMMIT. Add --author to MERGE() Dec 9, 2020
if sess.Username == "" || sess.Email == "" {
return nil, errors.New("commit function failure: Username and/or email not configured")

// Get the params associated with MERGE. We can share the commit argparser for now.
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a MergeParser now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's because the Merge in commands.go and the Merge in dfunctions.go support different arguments. This could trip up the HelpAndUsagePrinters function.

I am going to leave this as a TODO for now, and do some refactoring in a future PR.

go/libraries/doltcore/sqle/dfunctions/merge.go Outdated Show resolved Hide resolved
}

// Eval implements the Expression interface.
func (cf *CommitFunc) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
val, err := cf.Child.Eval(ctx, row)
dbName := ctx.GetCurrentDatabase()
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about code duplication from the other PR applies here. Do another pass on this in the near future to consolidate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Fix up tests

fix commit tests....

Add author param to merge

formatting...

Fix array prob
@VinaiRachakonda VinaiRachakonda merged commit a639e23 into master Dec 13, 2020
@VinaiRachakonda VinaiRachakonda deleted the vinai/add-commit-merge-params branch December 13, 2020 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants