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

Remove temporary files created in EDS tests #482

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

erlend-aasland
Copy link
Contributor

Clean-up post #474

Copy link
Collaborator

@sveinse sveinse left a comment

Choose a reason for hiding this comment

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

Looks good

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

I wanted to merge this, but I suddenly got stage fright: @acolomb what is the practice in canopen wrt. merging vs squash and merge? (I'm sorry that this Q is OT to the PR, do we have any forum for discussing best practices and policies for dev in canopen?)

@acolomb
Copy link
Collaborator

acolomb commented Jul 2, 2024

I don't know of any documented guidelines. I usually apply common sense and what has been known to work well from other projects. That's squash merges with hand-tuning on the commit message. It has the advantage of giving an easy reference to the PR number.

Usually the individual commits will be less interesting, but if they are cleanly separated, with good commit messages, and no cleanup was needed during review, I guess rebase-merging is fine, too.

@erlend-aasland
Copy link
Contributor Author

I wanted to merge this, but I suddenly got stage fright: @acolomb what is the practice in canopen wrt. merging vs squash and merge?

The Git log shows that merge commits have not been committed the last three years; squashed commits seems to be the prevailing style now, which I 100% fully support. Squashed commits with well-written commit titles/bodies makes for a readable Git history and increased maintainability.

@acolomb
Copy link
Collaborator

acolomb commented Jul 2, 2024

(I'm sorry that this Q is OT to the PR, do we have any forum for discussing best practices and policies for dev in canopen?)

Probably best to use GitHub's discussion tool for this. But with the currently small base of active contributors, it's not tragic to ask some OT questions within a PR :-)

@erlend-aasland
Copy link
Contributor Author

Following up in the OT track 😄 Is there an instant-messaging forum for CAN/CANopen stuff (Discord or similar)?

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

Squash merging is an opinionated 🔥 topic . Some love it, other loathe it, so I had to ask. What is "obviously, my friend" one place might not be that in other projects. YMMV.

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

Following up in the OT track 😄 Is there an instant-messaging forum for CAN/CANopen stuff (Discord or similar)?

I don't know of any, but I'm in 😎

@acolomb
Copy link
Collaborator

acolomb commented Jul 2, 2024

In general, this project is not as professionally managed, with all kinds of supporting infrastructure, as maybe other, bigger projects. I'm very grateful for all your contributions lately, @erlend-aasland and @sveinse. But I think time is better spent looking at the code and discussing asynchronously via GitHub discussions / issues / PRs, than on setting up some kind of developer hangout. I for one wouldn't have the opportunity to participate there regularly, thus working through notifications as time permits is my preferred workflow.

If the question was more generically about CAN and CANopen, not this library, then I don't know of any. That question might best be answered by CiA if they know of any such exchanges.

@erlend-aasland
Copy link
Contributor Author

Sure, I agree with all of that, @acolomb! Sorry, I did not imply that you (or any of the other maintainers) should use time and energy on such things.

@erlend-aasland
Copy link
Contributor Author

Anyway, let me know if you want any other changes before landing this :)

@sveinse sveinse merged commit 92a6e11 into christiansandberg:master Jul 2, 2024
1 check passed
@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

My first merge as maintainer in canopen, so I'm open to feedback if I did anything wrong @acolomb

@erlend-aasland erlend-aasland deleted the test/cleanup branch July 2, 2024 16:21
@erlend-aasland
Copy link
Contributor Author

Congrats on your first merge! I'm curious why you chose a merge commit:

  • the PR consisted of two commits, the latter being totally uninteresting in a Git log, only serving to cluttering the Git history
  • there has been no other merge commits the last three years; the de facto practice is squashed commits, as also confirmed by André
  • three (!) commits (the two from this PR and the merge commit) seems overly verbose for a trivial change

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

Oh, damn! Then I actually did fail. My intentions were to do a squash merger and I thought I did, but evidently I did not do that. So that's the rookie mistake then. For that I'm very sorry. 😢 (For the record: I'm not a git or github rookie, just new in this project).

@acolomb
Copy link
Collaborator

acolomb commented Jul 2, 2024

Life's not perfect. Neither is any project's Git history. Let's view it as a minor detail of such small importance that we can forget about it right away :-D

Sorry for not participating closely. Things got in the way this afternoon. But good to see we have one more co-maintainer to take care of stuff.

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