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

Add more tests for comments in lists #5104

Merged
merged 1 commit into from
Nov 27, 2021

Conversation

mujpao
Copy link
Contributor

@mujpao mujpao commented Nov 23, 2021

Fixes #4430 and #4420

@calebcartwright
Copy link
Member

Thanks! However, it'd be better to move the test files to a different, respective directories given that none of those files are directly associated with the current subdirectory (issue-4909) they're currently being proposed under

@mujpao
Copy link
Contributor Author

mujpao commented Nov 24, 2021

Do you mean that I should make a separate file or directory for each issue, or should I just rename the issue-4909 directory to comments-in-lists or something like that?

@calebcartwright
Copy link
Member

Do you mean that I should make a separate file or directory for each issue, or should I just rename the issue-4909 directory to comments-in-lists or something like that?

I think either of those will be fine. It's just that when we associate cases to specific issues we want to make sure the relationship is clear, and that's typically done via the file names, or directory names, or inline comments with issue references/links in larger/more general files

@mujpao
Copy link
Contributor Author

mujpao commented Nov 24, 2021

Is this right? I renamed the directory and added comments for each issue.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 24, 2021

Hey @mujpao, thanks for taking the time to rename those files. I personally think that the directory should either be named issue-4430 or issue-4420 to more directly associate these tests with the respective issues that this PR Fixes. Does this PR also solve 4909?

@mujpao
Copy link
Contributor Author

mujpao commented Nov 25, 2021

This PR just adds more tests. After working on #5091, I realized that #4420 and #4430 were also fixed by that PR, but I didn't think to add those specific test cases at the time. Since #4909, #4420, and #4430 were so closely related, I thought it would make the most sense to put all the associated tests in the same directory, but I could change it if needed.

@calebcartwright
Copy link
Member

I thought it would make the most sense to put all the associated tests in the same directory

That's reasonable and fine with me, but if we go that route then I'd like to see the containing directory renamed. I realize this may seem unnecessarily pedantic, but it's not uncommon for someone to need to go back to older issues and associated test cases so want to make sure the file and directory names don't cause any confusion.

@mujpao
Copy link
Contributor Author

mujpao commented Nov 27, 2021

I already renamed the directory to comments-in-lists. Should I have renamed it to something else?

@calebcartwright
Copy link
Member

I already renamed the directory to comments-in-lists. Should I have renamed it to something else?

Ah so you did, my apologies! That'll work thanks 👍

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

🚀

@calebcartwright calebcartwright merged commit ea042b9 into rust-lang:master Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line comments turned into block comments despite normalize_comments
3 participants