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

Ben merge analysis addition #253

Conversation

cactusbranch01
Copy link
Collaborator

Added new analysis notebooks and one new script to clone a repo into a repos2 folder.

@benedikt-schesch
Copy link
Owner

@cactusbranch01 Good job style checks are passing. It looks pretty interesting, but Jupyter notebooks are just for prototyping, sadly, not for final code. It would be great if you could convert them to .py files (+adjust them accordingly). You can copy/paste the code or there are commands in Jupyter to automatically transform to a .py file and then adjust it from there. If you have relevant outputs, please put them into some file inside a folder.

@mernst
Copy link
Collaborator

mernst commented Nov 15, 2023

@cactusbranch01 Let us know when you have addressed @benedikt-schesch 's comments. Thanks!

@benedikt-schesch
Copy link
Owner

@cactusbranch01 Also when you will write the code make sure the code use proper type hints in the function arguments and return values. For more info: https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html
In VS Code pylint checks for types and highlights errors if the types don't check out. (https://www.emmanuelgautier.com/blog/enable-vscode-python-type-checking)

@benedikt-schesch
Copy link
Owner

I recommend you run git merge origin/main while being on this branch such that you use the most updated version of the main branch. I added pre commit hooks that check style and other things before it creates a commit.

@cactusbranch01 cactusbranch01 marked this pull request as ready for review December 1, 2023 22:15
@cactusbranch01
Copy link
Collaborator Author

Do you mean to sync the fork before I send a pull request? Also, yes, I did run the style checks with "make style" and it looked like they scored 10/10. I'm just going to close this request, do that, then send another PR.

@benedikt-schesch
Copy link
Owner

Keep this PR open, I don't mean that. When you created this branch basically it was at a specfic version of main but main has changed now so you are no longer up to date with the latest code. There are two ways to fix this. 1) Rebase on main which basically adds all the missing commits but it's a huge mess bc you will have many merge conflicts (it's more clean though and ensures a more linear history). 2) Merge main into your branch, it will merge the current main into your branch and thus might only raise one single merge conflict. You should do 2 here.
Please read:
https://www.atlassian.com/git/tutorials/merging-vs-rebasing

@cactusbranch01
Copy link
Collaborator Author

Okay, gotcha. I think the main branch is up to date in my branch now. I appreciate the help, and just let me know if there is anything else I should do.

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.

3 participants