Skip to content

[Linter] Update files with respect to sort-imports rule#18567

Closed
bzhang0 wants to merge 5 commits intoAzure:mainfrom
bzhang0:issue-9252
Closed

[Linter] Update files with respect to sort-imports rule#18567
bzhang0 wants to merge 5 commits intoAzure:mainfrom
bzhang0:issue-9252

Conversation

@bzhang0
Copy link
Copy Markdown
Contributor

@bzhang0 bzhang0 commented Nov 8, 2021

This solves #9252. I've added the sort-imports rule into the ./common/tools/eslint-plugin-azure-sdk/.eslintrc.json and updated files appropriately after calling rush rebuild and rush lint.

I used the default settings for sort-imports, so if there should be changes to the rule settings, please let me know!

@ghost ghost added eslint plugin customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Nov 8, 2021
@ghost
Copy link
Copy Markdown

ghost commented Nov 8, 2021

Thank you for your contribution bzhang0! We will review the pull request and get back to you soon.

@bzhang0
Copy link
Copy Markdown
Contributor Author

bzhang0 commented Nov 8, 2021

Apologies, it seems like a different pull request #18565 is linked here as well. Perhaps this is because I solved the first issue in main instead of creating a new branch? Please excuse this, but if you know how to prevent this, please let me know so I can use that in the future! (I assume it is just making a new branch for every different issue).

@maorleger
Copy link
Copy Markdown
Member

maorleger commented Nov 8, 2021

Hi @bzhang0 - me again!

Yes, looks like your main branch contained some commits that are not part of this PR.

What I usually do is I keep my main branch empty of any changes. When I want to make a change I'll always make it in a branch. This way I can branch off of main at any point without anything sneaking in 😄

It looks like #18565 is based off of main and you have an open PR against so you don't want to blow those changes away yet. Here's what you can do instead:

  1. Create a new branch off of main
  2. in the new branch revert any commits that should not be there
  3. Cherry-pick or redo the commits that only matter for this changeset
  4. Open a new PR

Edit note: Changed instructions to avoid git reset and use git revert instead

There are other ways to tackle this if you don't want to redo changes - you can use git revert for example in this branch. I worry about merge-conflicts in this case since both commits apply to the same area of the codebase but if you can resolve those it could be easier.

If you have any questions, I'm happy to help - feel free to ping me!

@bzhang0
Copy link
Copy Markdown
Contributor Author

bzhang0 commented Nov 8, 2021

Hey @maorleger! Thank you for the feedback :)

I found out after I made my first pull request that I probably should have created a branch for each issue - I'm relatively new to using GitHub but always happy to learn :P.

Would it be okay to just delete my current fork and restart (creating two new branches for this and the previous PR)? I haven't made any changes that would take too long to fix if I started anew, I'm just not sure if there are any negative effects from deleting a fork. I feel like if I restart from a new fork, I can be more consistent in my PRs, changes, etc. Let me know if this is possible!

@maorleger
Copy link
Copy Markdown
Member

It'll be just fine with me 😄 I'm happy to review any new PRs you redo.

But, if I may make a recommendation - you don't need to delete your fork and fork again. If you wanted a nice clean slate you could the following:

git checkout main
git remote add upstream https://github.com/Azure/azure-sdk-for-js
git fetch upstream
git reset --hard upstream/main # Warning - destructive operation!
git push --force # Warning - destructive _github_ operation :) 

With all that said, feel free to do whatever is easier for you! I believe you can just delete and recreate your fork and it should be fine. Either way you will need to close these PRs and open new ones whenever you're ready.

Enjoy shaving yaks - we all have to do it!

@bzhang0
Copy link
Copy Markdown
Contributor Author

bzhang0 commented Nov 9, 2021

I'm having some issues following the method you suggested - for simplicities sake I think I will re fork and redo my fixes to this issue and #18565.

I will close this PR now, thank you for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization. eslint plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants