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

Fix needless operation #1687

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Conversation

Manvi-Agrawal
Copy link
Contributor

@Manvi-Agrawal Manvi-Agrawal commented Jun 29, 2024

fixes #1585.

@Manvi-Agrawal Manvi-Agrawal marked this pull request as ready for review June 30, 2024 07:34
@DmitryVasilevsky
Copy link
Contributor

DmitryVasilevsky commented Jul 8, 2024

I would prefer to limit this PR to operation -> function replacements only. Maybe with additional change to the output strings. I'd prefer other questions not in code comments, but rather in a GitHub issue. @swernli, what do you think?

@swernli
Copy link
Collaborator

swernli commented Jul 8, 2024

I would prefer to limit this PR to operation -> function replacements only. Maybe with additional change to the output strings. I'd prefer other questions not in code comments, but rather in a GitHub issue. @swernli, what do you think?

Yes, I agree. The questions in the comments can be removed and the PR focused on cleaning up the existing samples. We've had some debates about reducing some redundant samples, but this PR doesn't seem like the right time to take that on. @Manvi-Agrawal you are definitely asking the right questions though!

Copy link
Contributor

@DmitryVasilevsky DmitryVasilevsky left a comment

Choose a reason for hiding this comment

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

Please leave only operation -> function changes and output string improvements. Open issues for questions instead of adding comments (or add to existing ones).

@Manvi-Agrawal
Copy link
Contributor Author

Hi @DmitryVasilevsky and @swernli, created 2 separate issues for my questions : #1708 and #1709 . Could you please take a look at this PR?

Copy link
Collaborator

@swernli swernli left a comment

Choose a reason for hiding this comment

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

Thanks for splitting the general samples clean up into separate issues. The focused changes here look good!

Copy link
Contributor

@cesarzc cesarzc left a comment

Choose a reason for hiding this comment

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

Thank you for addressing this lints!

@swernli swernli added this pull request to the merge queue Jul 11, 2024
Merged via the queue into microsoft:main with commit efd6847 Jul 11, 2024
19 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.

[FollowUp] NeedlessOperation lint cleanup in samples not in list
4 participants