solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Fix Typographical Errors in Solidity Test Files

Open vtjl10 opened this issue 10 months ago • 8 comments

Description: This pull request addresses several typographical errors in the Solidity test files. The following changes have been made:

  1. Renamed modifer_recursive.sol to modifier_recursive.sol to correct the spelling of modifier
  2. Renamed no_target_for_abstract_constract.sol to no_target_for_abstract_contract.sol to fix the spelling of contract
  3. Renamed 142_inheritence_suggestions.sol to 142_inheritance_suggestions.sol to correct the spelling of inheritance

These changes improve the clarity and accuracy of the test files. Please review the changes and let me know if any further modifications are needed. Thank you!

vtjl10 avatar Feb 17 '25 08:02 vtjl10

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

github-actions[bot] avatar Feb 17 '25 08:02 github-actions[bot]

The changes themselves look ok, but if you're just fixing typos, please avoid unnecessarily spreading that over multiple PRs. I see you already submitted #15757 not long ago. I'll keep this one open for now in case you find something more you'd like to add.

I mean that's just 2 PRs from you specifically, but we're currently getting flooded with tons of small PRs that fix exactly 3 typos and stop, which looks like people are intentionally splitting them, making it more work to review them. This is unnecessarily drawing our attention from more important changes. We're generally fine merging even very small fixes as long as they are obviously correct, but if there are changes needed, we unfortunately can't spend too much time on reviewing them. Which is why e.g. your #15758 was stuck in limbo.

cameel avatar Feb 17 '25 14:02 cameel

The changes themselves look ok, but if you're just fixing typos, please avoid unnecessarily spreading that over multiple PRs. I see you already submitted #15757 not long ago. I'll keep this one open for now in case you find something more you'd like to add.

I mean that's just 2 PRs from you specifically, but we're currently getting flooded with tons of small PRs that fix exactly 3 typos and stop, which looks like people are intentionally splitting them, making it more work to review them. This is unnecessarily drawing our attention from more important changes. We're generally fine merging even very small fixes as long as they are obviously correct, but if there are changes needed, we unfortunately can't spend too much time on reviewing them. Which is why e.g. your #15758 was stuck in limbo.

updated, check please

vtjl10 avatar Feb 17 '25 15:02 vtjl10

Please also squash your commits a bit. Our convention is to have one logical change per commit and IMO fixing all those typos would be just one change. I'd only make sense create a separate commit for a specific typo if it was very widespread of if fixing it required some adjustments elsewhere and those would have to be grouped.

cameel avatar Feb 17 '25 15:02 cameel

Please also squash your commits a bit. Our convention is to have one logical change per commit and IMO fixing all those typos would be just one change. I'd only make sense create a separate commit for a specific typo if it was very widespread of if fixing it required some adjustments elsewhere and those would have to be grouped.

Got it, I'll keep that in mind. All checks have passed. Is this enough?

vtjl10 avatar Feb 17 '25 16:02 vtjl10

https://github.com/ethereum/solidity/pull/15873#discussion_r1958438804 is still unaddressed.

cameel avatar Feb 17 '25 16:02 cameel

#15873 (comment) is still unaddressed.

corrected

vtjl10 avatar Feb 17 '25 16:02 vtjl10

corrected

What do you mean? I see you merged develop into the branch, but I do not see any changes to the code. At least nothing addressing my comment.

Also, the branch needs a rebase on develop to remove the merge commits (we don't update them by merging develop into them as it makes history hard to follow). While doing this I'd also recommend squashing the commits.

cameel avatar Feb 18 '25 11:02 cameel

Closing for inactivity.

matheusaaguiar avatar Apr 16 '25 20:04 matheusaaguiar