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 Visual Studio for Mac from readme #807

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

Romfos
Copy link
Contributor

@Romfos Romfos commented May 5, 2024

  • Remove some not needed warning suppressions
  • Remove Visual Studio for Mac from readme

Visual Studio for Mac is deprecated
https://devblogs.microsoft.com/visualstudio/visual-studio-for-mac-retirement-announcement/

note: no changes in product, no need to release new nuget package

@Romfos Romfos changed the title Miscellaneous documentation updates Remove Visual Studio for Mac from readme May 5, 2024
Copy link
Member

@dtchepak dtchepak left a comment

Choose a reason for hiding this comment

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

nitpick (non-blocking): I think the PR description would be good to include in the commit message. It is a good explanation of the changes. 👍

@Romfos
Copy link
Contributor Author

Romfos commented May 6, 2024

nitpick (non-blocking): I think the PR description would be good to include in the commit message. It is a good explanation of the changes. 👍

done, but

I think this is very strange requirement because:

  1. After merge every commit have link to PR with full discussion
  2. Solution could be changed during review and have requirement to have it in commit message - very strange
  3. What if we have multiple commits?
  4. make sense to enable "squash" merge (will merge all commits in one with PR title as message). It could be enable in repo settings (+you can choose default merge method)

from my experience make sense to use commits for commits, pr for pr and do not mix conceptions

now commit message looks ugly =)

@Romfos Romfos requested a review from dtchepak May 6, 2024 10:23
@dtchepak
Copy link
Member

dtchepak commented May 8, 2024

The following is my opinion. Please take with a grain of salt 🧂 😄

  1. After merge every commit have link to PR with full discussion

My aim here is to have the repo largely independent from the host. If you checkout locally and go offline it would be good to have as much useful info as you can available to you. Also if we switched from github for some reason, it is nice if the code repo is the source of truth for a lot of this stuff. We do make some concessions for practicality like linking to MRs for relevant discussions, but I like the commits to stand on their own. (They don't have to show the discussion, just the end result of what is in the commit).

  1. Solution could be changed during review and have requirement to have it in commit message - very strange

The commit shouldn't match the MR description; it should describe the change. In this case the PR description just happened to match what was in the commit, but that info was missing from the commit message. (i.e. why was VS for Mac removed from readme? We know it was removed by looking at the diff, but what we can't tell is why)

If there is PR discussion that results in changes that will often result in a new commit with an appropriate message.

  1. What if we have multiple commits?

Each commit should summarise the main aims of/rationale for the changes made in that commit. Multiple commits is fine; each message will describe each commit's contents.

  1. make sense to enable "squash" merge (will merge all commits in one with PR title as message). It could be enable in repo settings (+you can choose default merge method)

We can do that. The default squash will include all commit messages. In a well-structured MR this should effectively describe the aggregate changes.

from my experience make sense to use commits for commits, pr for pr and do not mix conceptions

I agree with this. I think my request may have been confusing in this case as the MR description happens to be a close match for the commit contents. Main thing: commit message should describe stuff done in the commit that may not be obvious from a quick glance at the diff.

now commit message looks ugly =)

It is beautiful, thank you! 😂 🙇

@dtchepak dtchepak merged commit 4a5e6b6 into nsubstitute:main May 8, 2024
12 checks passed
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.

None yet

2 participants