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

Adapt to level config to match other Jupyter repos #237

Merged
merged 14 commits into from Dec 22, 2023

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 9, 2023

  • Adopts the .pre-commit-config.yaml hooks used by jupyter_client, including the Style & static checks from the Scientific Python Development Guide.
  • Switches to hatch for build backend and task runner.
  • Adds contributing document.
  • Removes use of deprecated .traverse() in favor of .findall().

tests/test_execute.py Outdated Show resolved Hide resolved
jupyter_sphinx/execute.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@12rambau
Copy link
Contributor

I was looking at the repository history, and it seems normal "merge" has been used so far. For this PR only "squash and merge" is available, is it also something you changed @blink1073 ?

Note that if it's something we can change I am more in favor of a normal merge as it gives more credit to big contributions to the repository than small fixes and as long as the naming convention is correctly managed, it still allow the use of a meaningful changelog (the one generated from Github works just fine)

@12rambau
Copy link
Contributor

@blink1073 I have reactivated the regular merge in the settings but I cannot set it back for this specific branch. Is it ok for you if I squash and merge so we can start from here for further development ?

@blink1073
Copy link
Member Author

I disabled "Allow merge commits" because they clutter up the history with merges from "main". Squash merging should be preferred IMO, unless there truly is some reason each commit in a PR has different meaning (which to me means that the PR should have been split). However, "Rebase merging" can be used in that scenario.

@blink1073 blink1073 merged commit c79b6e3 into jupyter:main Dec 22, 2023
11 checks passed
@blink1073 blink1073 deleted the update-config branch December 22, 2023 15:43
@12rambau
Copy link
Contributor

12rambau commented Dec 28, 2023

@blink1073 I got an upvote from @akhmerov so I think this matter could be discussed. On my side I still see a huge disadvantage to squash merging: It gives the same recognition to a typo PR and a huge feature PR. As an example, I was dead inside when I realized that these 2 PRs (pydata/pydata-sphinx-theme#540, https://github.com/pydata/pydata-sphinx-theme/pull/962/files) were given the same importance in my contributing section.

The problem you mention seems to be an egg and chicken issue as the "merge from main" conflict only occurs when you squash the previous PR. If not, git is very good at reconciling everything and Github provides a very convenient "sync" button that does everything for you from any branch.

Another example, I tried to contribute to a repository where people were less available for reviews (https://github.com/widgetti/ipyvuetify/pulls) but still using the same convention as the one you suggested: small single-feature PR. The consequence is that I have PR sitting there for 6 months preventing me from moving forward as I need them pushed sequentially. As this repository doesn't get as much maintainer love as the other Jupyter repos, I think it's very relevant to allow some motivated contributors to make big PR without squashing them.

And final arguments: it was the way things were done before here and everything seems ok so why changing it 😄 ?

@blink1073
Copy link
Member Author

I'm afk this week, and will defer to whatever you both think is best for this repo. My main point is that you can still see all the commits with the rebase option, but they are at least grouped. As long as the no-op "merge" commits aren't in the history I think it is fine.

@12rambau
Copy link
Contributor

12rambau commented Dec 29, 2023

No worries, I'll pile some small PR in the meantime and wait for your return for the bigger ones.
I tried to do it myself but I don't have access to branch policies and it seems the blocking parameter is coming from there.

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

Successfully merging this pull request may close these issues.

PendingDeprecationWarning: nodes.Node.traverse() is obsoleted by Node.findall()
3 participants