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

Take commit message as script argument #471

Merged
merged 10 commits into from
Feb 10, 2022

Conversation

kcranston
Copy link
Collaborator

@kcranston kcranston commented Dec 13, 2021

Addresses #466 and #311

Specifically:

  • takes a commit message as a string with the new "commit_message" argument for new_template and update_template
  • removes the previous boolean "custom_message" argument
  • if commit message not provided, uses "Initial commit" for new_template and "Updating assignment" for update_template

closes #471

@lwasser
Copy link

lwasser commented Jan 20, 2022

@nkorinek can you please review this?

@lwasser
Copy link

lwasser commented Feb 3, 2022

@nkorinek looking forward to potentially merging this and the other open pr today unless you found bugs?

@nkorinek
Copy link
Member

nkorinek commented Feb 3, 2022

Hey!

Small little bug I ran into I think. The argument --commit-message works fine! However, if you run it without that argument I got the following output.
image

Everything else in this PR seemed to work fine though!

@nkorinek
Copy link
Member

nkorinek commented Feb 3, 2022

To tack onto what I mentioned...if a template already exists and you run the argument with --mode delete or something like that, you don't get this message. It's only when creating a new template.

@lwasser
Copy link

lwasser commented Feb 3, 2022

oh that's interesting. Isn't it supposed to provide the default message "initial commit" when it's a new template? maybe that somehow broke with this updatE?

@nkorinek
Copy link
Member

nkorinek commented Feb 3, 2022

That's what I was thinking, I think the default somehow broke in this pr

@nkorinek
Copy link
Member

nkorinek commented Feb 3, 2022

When I run the code that throws the error message on the main branch I don't get the error

@kcranston
Copy link
Collaborator Author

Thanks @nkorinek - let me take a look and see what I broke.

@kcranston
Copy link
Collaborator Author

ok, added the default values in argparse so we should never get an empty message now! Current behaviour:

(abc-dev) course2022 $  abc-new-template assignment-01 --mode delete
Loading configuration from config.yml
Directory template_repos/assignment-01-template already exists for this course; deleting
                existing files but keeping .git directory, if it
                exists.
No changes to local repository.
Updating assignment list in config
Writing modified config at /Users/karen/Documents/courses-abc/course2022
(abc-dev) course2022  $ abc-new-template assignment-01 --mode delete --commit-message "first commit assigment1"
Loading configuration from config.yml
Directory template_repos/assignment-01-template already exists for this course; deleting
                existing files but keeping .git directory, if it
                exists.
No changes to local repository.
Updating assignment list in config
Writing modified config at /Users/karen/Documents/courses-abc/course2022
(abc-dev) course2022  $ abc-new-template assignment-01 --mode delete --commit-message
usage: abc-new-template [-h] [--commit-message COMMIT_MESSAGE] [--github]
                        [--mode {delete,fail,merge}]
                        assignment
abc-new-template: error: argument --commit-message: expected one argument

You get the standard argparse errror when you say you are going to provide a commit message but don't provide one.

@nkorinek
Copy link
Member

@kcranston passed the test! Seems to work properly now

@lwasser
Copy link

lwasser commented Feb 10, 2022

fixed a merge conflict! it's rebuilding. we can merge if it passes.

@nkorinek
Copy link
Member

Merging!!

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

Successfully merging this pull request may close these issues.

3 participants