search

django-cms / django-filer

File and Image Management Application for django
https://django-filer.readthedocs.io/
Other
1.73k stars 575 forks source link

Version 2.2 #1288

Closed jrief closed 2 years ago

jrief commented 2 years ago

Description

Collection of reviewed pull requests.

This PR makes up version 2.2.

Related resources

Checklist

codecov[bot] commented 2 years ago

Codecov Report

Merging #1288 (5996240) into master (9400ff7) will increase coverage by 0.13%. The diff coverage is 82.05%.

@@            Coverage Diff             @@
##           master    #1288      +/-   ##
==========================================
+ Coverage   72.06%   72.20%   +0.13%     
==========================================
  Files          71       72       +1     
  Lines        3229     3238       +9     
  Branches      561      563       +2     
==========================================
+ Hits         2327     2338      +11     
+ Misses        735      734       -1     
+ Partials      167      166       -1
Impacted Files Coverage Δ
filer/server/views.py 38.88% <0.00%> (ø)
filer/utils/files.py 69.87% <0.00%> (ø)
filer/templatetags/filer_admin_tags.py 86.56% <91.66%> (+0.41%) :arrow_up:
filer/__init__.py 100.00% <100.00%> (ø)
filer/admin/folderadmin.py 71.82% <100.00%> (+0.04%) :arrow_up:
...ile_owner_alter_file_polymorphic_ctype_and_more.py 100.00% <100.00%> (ø)
filer/models/foldermodels.py 86.47% <100.00%> (ø)
filer/models/abstract.py 81.51% <0.00%> (+1.68%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9400ff7...5996240. Read the comment docs.

vinitkumar commented 2 years ago

Mostly looks very good. Can we add more content to the changelog.rst as well. We can use something like git-changelog from brew install git-extras to generate a nicely formatted changelog file.

:shipit:

Aiky30 commented 2 years ago

Hi all, Can you please refrain from pushing big dumps of changes like this. Each feature / change should be a separate PR and merge. This is a release that contains changes, it shouldn't be a release that adds lots of changes, it's bad practice and doesn't follow good CI or release principles. A release is just that, not features and a release in one merge. This makes using git features such as history and blame really difficult too.

Trying to find which change here has broken djangocms-versioning-filer has taken a lot of my time today.

jrief commented 2 years ago

@Aiky30 They were all separate and approved pull requests.

Before releasing, I created a "merge" pull request which combined all of them (named version-2.2). This combined PR then was given for review and announced through our Slack channel. Nobody objected this, so I used it for a new release.

Aiky30 commented 2 years ago

@jrief What is the point of having separate pull requests if you're going to merge them in one commit? I strongly object to doing this and I believe it's bad practice. I am having to deal with a breaking change in this merge and because it's all merged in one commit I have no idea which "PR / Change" causes it. This also ruins commit history because all of the changes in this merge are labelled "Version 2.2", absolutely meaningless to anyone looking at the git history. A version didn't introduce a change, a change did. The only way that you see that in Git history is by merging changes separately.

If I saw the message yesterday I would have objected.

I recommend reading this, especially the first sentence: "Additionally, having periodic checkpoints means that you can understand how you broke something." https://sethrobertson.github.io/GitBestPractices/

jrief commented 2 years ago

@Aiky30 You can see all pull requests making up this combined pull request here: #1288

Aiky30 commented 2 years ago

@jrief I think you're missing the point. The git history and blame features are now useless. I am someone now trying to identify which "change" in that commit caused an issue. The name is useless, it doesn't describe the change.

An example of good commit history where the changes are features / fixes / releases and squashed. It's clear to anyone looking at the history what code changes have happened. It also clearly separates concerns: https://github.com/django-cms/django-cms/commits/develop-4

An example of a file using blame where the changes are clear: https://github.com/django-cms/django-cms/blame/develop-4/cms/api.py

For the change in this merge all you will see everywhere is "Version 2.2", not "feat: Refactor Folder permissions" on the places that this change matter, or "fix upload invalid SVG files.

As you can probably tell I am against merging things in bulk like this.