LibreSolar / libresolar.github.io

Libre Solar website built with VuePress
https://libre.solar
Creative Commons Attribution Share Alike 4.0 International
4 stars 9 forks source link

dependency version fixes & add "Improve this site" link & README: add dev instructions #8

Closed hoijui closed 5 years ago

hoijui commented 5 years ago

see commit messages. except maybe the "add link to edit page source on github" one, it should not be very controversial.

martinjaeger commented 5 years ago

Cherry-picked dev instruction improvements. Edit link will not be added at the moment.

hoijui commented 5 years ago

@martinjaeger why did you make 1 commit out of 4? I would recommend not to do that whenever possible, as it not just makes the history less granular, but also changes the commits official/visible ownershi, in this case from me to you. otherwise: thanks! :-)

martinjaeger commented 5 years ago

I wanted to include the changes for dev instructions, but they were combined into one PR with the edit link, so I couldn't just merge the PR. Cherry-picking unfortunately removed ownership. Sorry, I didn't find an easy way to preserve them except for linking to this PR.

I think the granularity doesn't need to be too fine, e.g. several small changes in the Readme file if they belong to the same topic. In such cases I prefer to squash merge a PR to improve readibility of the commit history.

hoijui commented 5 years ago

the history now contains less information. It is more readable only in the sense that it takes less time to read. Many people think of the git history as something like a changelog. A changelog is a log of changes between versions of a software/hardware, meant for the eye of users of that project. git history is not for the public; it is for the developers - and as such - more granularity is better for practical reasons (cherry picking, reverting, bisecting, following the though process, ...).

cherry-picking does not change authorship of commits. smashing them into one commit does change ownership. instead of that, you can merge the three commits into master, which still leaves them as one commit in history, but still visible as 3 when browsing, bisecting, reverting and so on. More ideal though (in this case), would be to cherry-pick them individually onto master, as it leaves the history in a more "linear look", and it would be viable because they are rather small maintenance changes then a single, big feature/change.

These commits do not deal with the same topic, and fixes especially should always be separate, if they functionally can. What if my fix turns out to be bad, and has to be reverted? you now would have to manually create a commit for that, because you are not able to revert it as such without loosing the other changes.

you may want to read about "atomic commits" (or see section 2.3.2 and 2.4.1 of C4.2, but that might be overkill)

I managed some quite active commnity projects (software), and was responsible for the git handling for a year in a middle sized project (~ 5 - 10 commits per day). anyone thereabout and in bigger projects will tell you, that commits can't be too small. as long as functionally possible, make separate commits. it might not seem so important in this project with a relatively low activity in terms of commits per time, but even here it is good to keep it up for practise, for a later stage of the project, and for training the devs habbits in other projects.

hoijui commented 5 years ago

.. yep, that's how to spend a saturday! arguing on the internet! sorry @martinjaeger martin, don't feel oblidged to waste your time aswell. ;-)

hoijui commented 5 years ago

I just learned now, that one should make separate branches for patches when using pull requests on github. ;-) ouh well!

martinjaeger commented 5 years ago

Just coming back from vacation, sorry for the late response.

Seems like I didn't use cherry-pick properly. Sorry for that. Actually it was the first time I needed it because I wanted to merge only a part of a PR.

I agree that atomic commits make sense. But I don't agree that commits can't be too small. For example if a new function of a software is implemented and as a second step the documentation is updated to reflect that change, I prefer to have both changes in one commit because the changes belong together. Maybe I was wrong, but in this case I thought that changing the Readme and the gemspec belong together, so I combined them to one commit.

It's not the case here, but I don't want any commits in the history of e.g. a software project just saying "Readme updated" following a commit "Function xyz implemented", as it makes the commit history longer and as such makes it harder to find the commit where something was changed.

And I also think that merge commits often decrease readibility. E.g. if someone sends a PR with a simple commit "Bug xyz fixed", merging of the PR creates an additional commit "Merge pull request ... from ...". Unfortunately, rebasing doesn't preserve ownership (at least if done via the button on github), so I would squash merge such a PR to have only one commit message. What do you think about this approach? Is there a better way to avoid two commit messages in the history for only one line of code changed?

martinjaeger commented 5 years ago

I just learned now, that one should make separate branches for patches when using pull requests on github. ;-) ouh well!

I think that's only needed if the collaborators have write privileges to a repository, e.g. in the team of an organization. In case of an external collaborator, the fork acts as the separate branch.

hoijui commented 5 years ago

...In case of an external collaborator, the fork acts as the separate branch.

... that is how I thouhg it should be too, but actually it is not true with GitHub!

  1. fork a project on github
  2. checkout master locally
  3. make a few commits
  4. push to the fork on github
  5. make pull-request
  6. make more commits locally
  7. push to fork on github

the pull request made in step 5 now contains both the commits from steps 3 and 6

this allows to easily update a pull request after it was created, so it makes sense, but one should always add a step between 2 and 3: git checkout -b patch_1 ... if one might continue writing commits for the project after the pull request.

martinjaeger commented 5 years ago

I see. So it makes sense to branch before sending a PR, you are right.

hoijui commented 5 years ago

I agree that atomic commits make sense. But I don't agree that commits can't be too small. For example if a new function of a software is implemented and as a second step the documentation is updated to reflect that change, I prefer to have both changes in one commit because the changes belong together. Maybe I was wrong, but in this case I thought that changing the Readme and the gemspec belong together, so I combined them to one commit.

It's not the case here, but I don't want any commits in the history of e.g. a software project just saying "Readme updated" following a commit "Function xyz implemented", as it makes the commit history longer and as such makes it harder to find the commit where something was changed.

+1, you right!

except that longer commit history is in practice not a problem, if one uses a search tool.. ahh.. do you checkout history on github itself? then it would be unhandy indeed. it is not designed for searching.

And I also think that merge commits often decrease readability. E.g. if someone sends a PR with a simple commit "Bug xyz fixed", merging of the PR creates an additional commit "Merge pull request ... from ...". Unfortunately, rebasing doesn't preserve ownership (at least if done via the button on github), so I would squash merge such a PR to have only one commit message. What do you think about this approach? Is there a better way to avoid two commit messages in the history for only one line of code changed?

I agree, I also don't like merge commits. They make sense only for bigger change-sets, those one usually develops in a separate branch with a specific name, different form the origin branch.

If you go to the project settings and scroll down to "Merge Button", you can disable merge commits and squash merging.

btw, when merging/rebasing locally instead on github, the setting related to this is called "fast-forward". git merge --no-ff ... meas, there will always be a merge commit, while git merge --ff-only ... means there will not be one, but the command might fail if fast-forward is not possible.

NOTE: you probably already know about it, but if not, I recommend a lot reading about/trying git rebase -i. it is very nice to cleanup the local git history.