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

[DNNL] Fix end of line in test_dnnl UT file #11560

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

billishyahao
Copy link
Contributor

The end of line in test_dnnl.py is CRLF which is not identical to other python files. This patch is going to fix this by replacing CRLF to LF.

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

@billishyahao
Copy link
Contributor Author

billishyahao commented Jun 3, 2022

Hi @masahi, @AndrewZhaoLuo , Could you please review this change?

@masahi
Copy link
Member

masahi commented Jun 3, 2022

Why do we need to fix this?

@AndrewZhaoLuo
Copy link
Contributor

If I understand this change correctly this is entirely style; git will handle different line deliminators fine, but the issue is that if a windows dev edits a file with LF, it will replace all lines with CRLF through the entire file which means every line will be changed in diffs. This will make the commit log a bit gummier.

cc @areusch @driazati it seems like a good idea to force LF for new lines? can we lint for this.

@driazati
Copy link
Member

driazati commented Jun 7, 2022

We should definitely check for this kind of thing in CI, opened #11609

@billishyahao
Copy link
Contributor Author

Hi @masahi , @AndrewZhaoLuo , I hit this issue when I am trying to accomplish my another PR #11513 . It seems that somebody use windows as development environment and unexpectedly reformat the whole LF to CRLF. Git status command shows this change and it is annoying for other developer of tvm. To eliminate this issue, I propose this patch.
As Andrew 's words, it is better to enable lint to avoid other people to change LF to CRLF unexpectedly.
@AndrewZhaoLuo Hi, may I continue merging this change?

@masahi masahi merged commit a95a820 into apache:main Jun 8, 2022
Kathryn-cat pushed a commit to Kathryn-cat/tvm that referenced this pull request Jun 10, 2022
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