earthlab / abc-classroom

Tools to automate github classroom and autograding workflows
https://abc-classroom.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
29 stars 21 forks source link

function init_and_commit and custom commit messages #466

Closed lwasser closed 2 years ago

lwasser commented 2 years ago

I believe this has come up before somewhere related to the default text editor and git.

I am working on tests to cover init_and_commit with a default message vs custom. when you add a custom message, i thought it would allow me to provide a custom message and then use that in the commit. Instead it:

  1. accepts the custom message
  2. opens up a default text editor in my terminal (is that vi or something it's not my default text editor)
  3. waits for a message This is tricky to test but the bigger issue is i expected that i could just provide it with text to customize the commit rather than having to go to a text editor.

Is there a use case where someone would want to initiate a text editor vs just providing the message via a parameter? especially if we use this as a package i think it might be good to rethink this before i keep moving on this test.

custom_message : str (defaults to default message - 'Initial commit')
        String with a custom message if you wish the first commit to not be
        the stock git init message provided by abc-classroom.

possibly related to https://github.com/earthlab/abc-classroom/issues/311

update. i believe the editor is coming from here:

def get_commit_message():
    default_message = """
    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    # This message will be used as commit and Pull Request message."""
    message = input_editor(default_message)

so i think we should talk about how custom message should be used. i also want to figure out where someone would actually add a custom message as well (ie create_template() is the only spot where they may want to create a custom message. i am not sure that we really need to open an editor for this step but do want to see what others think. what would an editor provide that we couldn't provide by just allowing a string input instead of a boolean?


One last note here - this function also has a sys.exit() call which will stop and reset the python kernel. We may want to refactor that behavior as well to fail gracefully so this can act in a package envt (e.g. run in a notebook)

kcranston commented 2 years ago

I don't see why we could not modify the current behaviour to take the message as an argument rather than open an editor. The command line scripts would then work like this:

abc-new-template assignment-name --custom-message "custom git message here"

and

abc-update-template assignment-name --custom-message "custom git message here"

For add, a default message of "Initial commit" and for update a default message of "changing assignment"?

This goes against the git ethos of informative commit messages, but if you are ok with that, I can implement this.

kcranston commented 2 years ago

I wonder about renaming that option to commit-message rather than custom-message to make it clearer.

lwasser commented 2 years ago

@kcranston def think making the parameter commit-message makes sense. and then in terms of the informative messages - are you referring to the default message OR if we remove the editor it may encourage less verbosity of comments because only the commit message will be added?

Right now i think there is initial commit for all of our template repos. I agree it's not informative - we could encourage the user to use commit-message perhaps if that is what you are concerned about? I def don't want to encourage bad practices. We could also force commit-message as a parameter too and have the default options be an argument if that makes sense.

my main concern is the inputeditor() calling what i think is the default editor rather than the users chosen editor and also testing that isn't trivial altho i can do it.

kcranston commented 2 years ago

What if we had a default commit message for abc-new-template ("initial commit") and forced a commit message for abc-update-template by making --commit-message a required argument?

This would allow us to 1. have informative commit messages when updating an assignment repo and 2. eliminate calling the text editor completely (and letting us close #311 )

lwasser commented 2 years ago

i'm ok with this if you are! @kcranston it would also make testing a lot more streamlined but from a user perspective i really did expect message to allow me to provide a message that it would then use as a commit. i will hold off on writing any more tests for git.py until we implement this.

so

  1. initial commit can stay as a default for the first commit
  2. we force users to provide a commit for any updates this makes a lot of sense to me. and will avoid the oddness of VI or whatever editor opens by default on the users machine. i think it was vi for me as :wq worked.
lwasser commented 2 years ago

i forgot about this process with earthpy. as you write tests it makes you rethink the entire package structure and you fix things and improve. it's kind of a neat process to double check work that was previously done via writing tests.

nkorinek commented 2 years ago

https://github.com/earthlab/abc-classroom/pull/471 got merged so closing this too!