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

Cookiecutter workflow 1. initialize cookiecutter (Step 1-11.ii) #57

Merged
merged 9 commits into from
Aug 7, 2024

Conversation

bobleesj
Copy link
Contributor

@bobleesj bobleesj commented Aug 7, 2024

Step 11. Add and commit each of the untracked files to the git repo. These are in cookiecutter but not in the original repo, so can simply be "git added". Do it one (or a few) at a time to make it easier to rewind by having multiple commits.

I followed the instructions step-by-step. I am in an uncomfortable zone, but I have faith moving forward.

  • Moved the .git file to the cookiecutter repo
  • Created the cookierelease branch
  • Copied doc files
  • Copied src files
  • Commit after tracking similar files in a batch

@sbillinge

@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 7, 2024

Screenshot 2024-08-06 at 11 19 53 PM

Source of faith.. ran from the cookiecutter repo.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

This looks great!

A couple of comments....I see things being lost in the run.txt that I guess should probably be there...is there a reason for this? Also, in general, this is quite a lot on a single PR. The files that are aded with no changes are easier to merge than ones that are modified.

Finally, a good practice is to review your own PR on GH, in case you are not. When I see the requirements/README.txt I guess you are not....

But small points....this is going great!

requirements/run.txt Outdated Show resolved Hide resolved
@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 7, 2024

A couple of comments....I see things being lost in the run.txt that I guess should probably be there...is there a reason for this?

Thanks for the catch. Now, the requirements/run.txt content is simply copied to the cookiecutter repo.

Also, in general, this is quite a lot on a single PR. The files that are aded with no changes are easier to merge than ones that are modified.

Got it - I will try to separate moving files vs. modifying files in separate PRs moving forward.

Finally, a good practice is to review your own PR on GH, in case you are not. When I see the requirements/README.txt I guess you are not....

Indeed. I was very excited after the tests were passed and made a pull request. The README.txt file is now removed.

Any other feedback? I am learning much from you.

@sbillinge

@sbillinge
Copy link
Contributor

This looks good, more or less ready to merge, but it is failing tests now.

@sbillinge sbillinge merged commit 1611d94 into diffpy:cookie Aug 7, 2024
2 of 3 checks passed
@sbillinge
Copy link
Contributor

@bobleesj I went ahead and merged this because I noticed that the code has not been moved to a src directory yet, so that will break tests anyway. We may as well fix tests when the code structure is correct.

@sbillinge
Copy link
Contributor

Finally, a good practice is to review your own PR on GH, in case you are not. When I see the requirements/README.txt I guess you are not....

Indeed. I was very excited after the tests were passed and made a pull request. The README.txt file is now removed.

it was correct to make the PR, but after making it, you can save time by going to GH and reviewing it yourself. You will be seeing the same diffs I will see and you can fix things you missed by pushing fixes before I even look at it. You would have seen the readme saying "delete me", for example.

@bobleesj bobleesj deleted the cookierelease branch August 7, 2024 17:04
@bobleesj bobleesj restored the cookierelease branch August 7, 2024 19:31
@bobleesj bobleesj deleted the cookierelease branch August 7, 2024 19:32
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.

2 participants