TE
科技回声
首页24小时热榜最新最佳问答展示工作
GitHubTwitter
首页

科技回声

基于 Next.js 构建的科技新闻平台,提供全球科技新闻和讨论内容。

GitHubTwitter

首页

首页最新最佳问答展示工作

资源链接

HackerNews API原版 HackerNewsNext.js

© 2025 科技回声. 版权所有。

Git Things

196 点作者 nalgeon超过 1 年前

22 条评论

ender7超过 1 年前
&gt; A better approach is to optimistically merge most changes as soon as not-rocket-science allows it, and then later review the code in situ, in the main branch. And instead of adding comments in web ui, just changint the code in-place, sending a new PR ccing the original author.<p>This may work for small projects or open source projects that generally receive high-quality PRs, but this sounds truly infeasible for large teams or organizations.<p>There are a couple reasons why the code review process occurs before merging. First, it helps keep the canonical version of the codebase in a &quot;correct&quot; state. This is, ironically, an extension of the &quot;not rocket science&quot; principle that the article mentions. Without this invariant, any checkout of the codebase might contain what is essentially &quot;WIP&quot; code that will waste other engineers&#x27; time if they have to interact with it. Second, the social pressure of blocking someone else&#x27;s work is an important and necessary force for motivating code review. The idea that folks will go back and review code that&#x27;s already been merged is akin to &quot;will add tests in a followup PR&quot;. It&#x27;s a pleasant lie we tell ourselves, but it rarely comes true.<p>Relatedly, the article also seems to suggest that the code REVIEWER should be providing the changes necessary after code review. This is problematic for so many reasons: first, it places an even higher burden on code reviewers&#x27; time which no one wants; second, it encourages poor code hygiene if someone else is just going to fix up your crappy work later on; third, it robs junior engineers of what is perhaps their most valuable learning experience: getting feedback from more experienced engineers and acting upon it.<p>(there are other issues here; the general thesis of the article leans heavily towards making code easier to _write_ rather than _read_, which again I think it just not appropriate for a codebase with a significant lifetime or number of contributors)
mfrw超过 1 年前
&gt; Second, not every change needs a great commit message. If a change is really minor, I would say minor is an okay commit message!<p>I want to slightly disagree on this for folks who are early on in career e.g me; I think there is a lot of value in learning to communicate precisely and having a habit of writing good commit message has certainly improved my skills, both verbal and written.<p>Even though, this activity appears futile for a minor change; it might help to improve other skills :)
评论 #38830991 未加载
评论 #38832786 未加载
评论 #38830774 未加载
评论 #38832174 未加载
评论 #38831262 未加载
gverrilla超过 1 年前
I&#x27;m a beginner dev and I find it very odd both that vscode tries to limit my commit messages to 50 or 55 characters, and also that python linters want to make my lines of code so small. What&#x27;s the reason behind this? Old small monitors of the past?<p>I also don&#x27;t get why fediverse, mastodon seem to limit characters on a post, mimicking the stupid twitter, which is a niche social network. I way be wrong about the limit (never tried), but these platforms feel for ignorant outsiders like twitter-clones with a different infrastructure.
评论 #38831568 未加载
评论 #38831166 未加载
评论 #38832068 未加载
评论 #38831329 未加载
评论 #38831193 未加载
评论 #38831134 未加载
评论 #38831811 未加载
评论 #38831691 未加载
评论 #38831630 未加载
评论 #38831145 未加载
评论 #38838672 未加载
评论 #38831523 未加载
评论 #38831282 未加载
kissgyorgy超过 1 年前
<p><pre><code> Instead, adjust the asserts such that they lock down the current (wrong) behavior, and add a clear &#x2F;&#x2F; TODO: comment explaining what would be the correct result. This prevents such tests from rotting and also catches cases where the behavior is fixed by an unrelated change. </code></pre> Don&#x27;t do that, because they are still comments and even worse, the tests are lying. In pytest, there is a way to tell that those tests are expected to fail: xfail, so it will show in the test results every time and it&#x27;s clear it&#x27;s just a temporary things and should be fixed.<p><a href="https:&#x2F;&#x2F;docs.pytest.org&#x2F;en&#x2F;7.1.x&#x2F;how-to&#x2F;skipping.html#xfail-mark-test-functions-as-expected-to-fail" rel="nofollow">https:&#x2F;&#x2F;docs.pytest.org&#x2F;en&#x2F;7.1.x&#x2F;how-to&#x2F;skipping.html#xfail-...</a>
lexicality超过 1 年前
&gt; Second, not every change needs a great commit message. If a change is really minor, I would say minor is an okay commit message!<p>It&#x27;s very hard to rank them, but very high on my list of things that make me want to send people actionable threats is when I&#x27;m searching down the history of a bug to figure out if it was a deliberate change that had unexpected consequences or just a mistake is when the git blame path ends on a commit like this.<p>I don&#x27;t care how minor you think your change is, if you&#x27;re working on something collaboratively you need to express intent in your commit messages!
评论 #38831302 未加载
评论 #38831641 未加载
评论 #38832803 未加载
评论 #38831362 未加载
评论 #38835389 未加载
barbegal超过 1 年前
&gt;if you want to reliably record file moves during refactors in git, you should do two commits: the first commit just moves the file without any changes, the second commit applies all the required fixups.<p>Yes this will record a file move in one of the commits but if you diff between before and after the two commits a file move might not be shown. When thinking about file moves in git it is worth remembering this comes from the diff tool not from the commits since the commits just show the state of the file system at the point it is committed.
评论 #38830776 未加载
cesnja超过 1 年前
Due to (async) code reviews slowing down the development, we&#x27;ve adopted some workflows described at <a href="https:&#x2F;&#x2F;martinfowler.com&#x2F;articles&#x2F;ship-show-ask.html" rel="nofollow">https:&#x2F;&#x2F;martinfowler.com&#x2F;articles&#x2F;ship-show-ask.html</a> and it&#x27;s working out pretty great. The committer decides how many approvals they need to merge a pull request, based on how confident they are about the code they wrote. This allows us to quickly merge small incremental improvements that would otherwise be bundled in a crowded feature pull request.
评论 #38830599 未加载
weavejester超过 1 年前
Why bother keeping minor commits in feature branches? They just clutter the main branch without adding any useful information that a future maintainer could use.
评论 #38831428 未加载
评论 #38831033 未加载
kingkongjaffa超过 1 年前
&gt; That’s why for typical project it is useful to merge pull requests into the main branch — the linear sequence of merge commits is a record of successful CI runs, and is a set of commits you want to git bisect over.<p>This is kind of obvious after reading but no one ever explained this to me until now
kageiit超过 1 年前
I feel this advice has some merit when dealing with oss projects, systems&#x2F;backend or small teams.<p>It tends to break apart when working with large teams or when working on mobile apps where one can&#x27;t just rollback a change easily after it&#x27;s shipped. The descriptions need to be more detailed and capture lot more information.<p>For example, our team would require all mobile devs to add information about feature flags for each change to turn the feature off if things go wrong. This also places additional review burden to make sure the flag covers all the new code introduced and does not interact in a bad way with other existing feature flags
dclowd9901超过 1 年前
&gt; When fixing a bug, add a failing test first, as a separate commit. That way it becomes easy to verify for anyone that the test indeed fails without the follow up fix.<p>I would genuinely like to know in what world this is useful advice. Maybe in personal projects or small teams or companies with a maniacal lead?<p>I just can’t imagine someone in a sufficiently large organization (which most companies hope to be someday) goes sifting through feature commits to get to the bottom of a failing test.
评论 #38830930 未加载
评论 #38830951 未加载
评论 #38831956 未加载
评论 #38830827 未加载
评论 #38832333 未加载
blep_超过 1 年前
The 50-character summary thing is bizarre. As far as I can tell, what happened is someone did statistics on summary lines, found that the average was around 50, and then louder people with strong opinions missed the word &quot;average&quot; and declared 50 as a maximum.<p>My hot take is that you should use exactly as many characters as you need to write a meaningful summary, and people who insist on using tiny terminal windows can deal with the consequences of their actions.
评论 #38830699 未加载
评论 #38831394 未加载
评论 #38831447 未加载
pjbster超过 1 年前
It feels like there&#x27;s a tiny bit of conflicting advice in this. There&#x27;s the &quot;not rocket science&quot; rule which results in &quot;no CI-failing commits on main&quot; and then there&#x27;s this:<p>&gt; Third, our review process is backwards. Review is done before code gets into main, but that’s inefficient for most of the non-mission critical projects out there. A better approach is to optimistically merge most changes as soon as not-rocket-science allows it, and then later review the code in situ, in the main branch.<p>But the tip about adding a failing test as a separate commit on the feature branch wouldn&#x27;t survive a merge and it wouldn&#x27;t live long enough to be reviewed either.<p>I like most of the advice in this article but this review one is giving me pause.
评论 #38831404 未加载
aliceryhl超过 1 年前
Regarding the review process ... one thing that I find challenging and don&#x27;t know a good solution to is documentation. I&#x27;ve received many PRs where the change itself is fine, but the PR is dragging out because the documentation is lacking, and getting the PR author to improve it sometimes takes a lot of review rounds.<p>What would you do to avoid this?<p>Sometimes the same situation comes up with tests, but it is not as common in my experience.
评论 #38831543 未加载
评论 #38831830 未加载
评论 #38831537 未加载
hmeh超过 1 年前
&gt; However, in a typical project, landing a trivial change is slow. How long would it take you to fix it&#x27;s&#x2F;its typo in a comment? Probably 30 seconds to push the actual change, 30 minutes to get the CI results, and 3 hours for a review roundtrip.<p>It doesn’t have to be like this. This is a choice. There’s no reason a team cannot work towards a world where they can push a small fix to master directly after running their 5 second test suite on their local machine.<p>We are used to squalor, but it’s not necessary. It’s the result of a series of choices. There are other ways to do our work that maintain continuity of productivity. Old projects can feel like new projects.<p>Any time someone writes something like this, they are normalizing slights against our fellow developers. They may not recognize it as such because it’s all they’ve ever known, but we should (collectively) know that better is possible and strive for it.
评论 #38831976 未加载
epage超过 1 年前
&gt; Within a feature branch, not every commit necessary passes the tests (or even builds), and that is a useful property! Here’s some ways this can be exploited:<p>&gt; When fixing a bug, add a failing test first, as a separate commit. That way it becomes easy to verify for anyone that the test indeed fails without the follow up fix.<p>As a reviewer, I want the first commit to be passing and then the bugfix commit update the test to still pass<p>- Makes change of behavior obvious to reviewer<p>- Assumeing all commits are tested, ensures the test is testing the right thing
Aeolun超过 1 年前
&gt; Instead, adjust the asserts such that they lock down the current (wrong) behavior, and add a clear &#x2F;&#x2F; TODO: comment explaining what would be the correct result. This prevents such tests from rotting and also catches cases where the behavior is fixed by an unrelated change.<p>Yuck! Tests asserting wrong behavior?! Then you might as well not have the thing. Just remove it entirely.
评论 #38831640 未加载
taf2超过 1 年前
I really like:<p>[diff] colormoved = &quot;default&quot; colormovedws = &quot;allow-indentation-change&quot;<p>That is great for understanding the actual code changes wish it was on by default.
quickthrower2超过 1 年前
I am a merger not a rebaser but it is probably a tab&#x2F;space level personal quirks distinction!
评论 #38831937 未加载
bornfreddy超过 1 年前
&gt; Instead, adjust the asserts such that they lock down the current (wrong) behavior, and add a clear &#x2F;&#x2F; TODO: comment explaining what would be the correct result.<p>This is awful advice. The test is there to prevent regression. Why would you change it to not only allow the wrong behavior, but to enforce it? Either fix the test or decide that you won&#x27;t, in which case remove it. If neither option appeals to you then mark it as optional and let CI succeed with a warning. But changing the test to lock the wrong behavior? No, just no.
forrestthewoods超过 1 年前
I can’t help but feel that the fact that rebase and merge are treated as different commands is broken. This topic keeps coming up on HN lately and it feels like the framing is all wrong. I dunno.<p>At the end of the day it’s all just a sequence of commits and some commit labels.
90319080超过 1 年前
Wax