Skip to content

Conversation

@parhamsaremi
Copy link
Contributor

  • add fix command option
  • add fix --check option for not altering the code

@knocte
Copy link
Collaborator

knocte commented Aug 1, 2022

From the 3 commits existing in this PR, is there any reason to separate commit 1st and 2nd?

@parhamsaremi
Copy link
Contributor Author

Ummm I remember the first commit being only fixes to what janus wrote. the second commit is what you tasked me to do 🤔

@knocte
Copy link
Collaborator

knocte commented Aug 1, 2022

the second commit is what you tasked me to do 🤔

Separation is not needed just because one commit is from one author and another commit from another. (Besides, did you keep his code in the end or you rewrote from scratch?). If there's a need to squash 2 commits from different authors, you just use the tag Co-authored-by: Jon Doe <[email protected]> at the end of the commit message.

@parhamsaremi
Copy link
Contributor Author

the second commit is what you tasked me to do thinking

Separation is not needed just because one commit is from one author and another commit from another. (Besides, did you keep his code in the end or you rewrote from scratch?). If there's a need to squash 2 commits from different authors, you just use the tag Co-authored-by: Jon Doe <[email protected]> at the end of the commit message.

In the end, I used his code. I think I should merge commits then 🤔

@knocte
Copy link
Collaborator

knocte commented Aug 1, 2022

In the end, I used his code. I think I should merge commits then

Sure let's do it. BTW let's rather use the word "squash", not "merge", to not confuse with the "git merge" command (which we should almost never use btw) :)

@parhamsaremi
Copy link
Contributor Author

In the end, I used his code. I think I should merge commits then

Sure let's do it. BTW let's rather use the word "squash", not "merge", to not confuse with the "git merge" command (which we should almost never use btw) :)

yeah you are right :D

@parhamsaremi parhamsaremi force-pushed the quick-fix-exitcode-squashed-V2 branch from ae30b53 to 70b3a4c Compare August 1, 2022 08:31
@knocte
Copy link
Collaborator

knocte commented Aug 1, 2022

Co-authored-by: parhamsaremi [email protected]

Nit, the bit before the e-mail address should be in Name Surname format, not username. Unless you have anything against that?

@parhamsaremi
Copy link
Contributor Author

Co-authored-by: parhamsaremi [email protected]

Nit, the bit before the e-mail address should be in Name Surname format, not username. Unless you have anything against that?

hmmmm OK sure thinking I don't have anything against it, I just preferred my username, but since my username is my Name and Surname I don't see any problem switching.

janus and others added 2 commits August 3, 2022 13:56
Add --check functionality to fix command. When passed will ignore fixes and will only check if passed rule has suggested fixes.
@parhamsaremi parhamsaremi force-pushed the quick-fix-exitcode-squashed-V2 branch from 59db421 to 4392044 Compare August 3, 2022 09:29
@parhamsaremi parhamsaremi marked this pull request as draft November 3, 2022 13:37
@parhamsaremi parhamsaremi changed the title add fix command line option and fix --check DRAFT: add fix command line option and fix --check Nov 3, 2022
@webwarrior-ws
Copy link
Contributor

I've looked closely at SuggestedFix type.
And it has 2 fields that are supposed to serve the same purpose:

/// Text to be replaced.
FromText

and

/// Location of the text to be replaced.
FromRange

It's not stated anywhere how these fields are supposed to be used.
In practice, TestRuleBase.ApplyQuickFix uses only FromRange.
This PR uses only FromText (that's buggy because there might be other occurrences of FromText in source that are not meant to be replaced).

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.

4 participants