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.

Sign up for GitHub

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

Jump to bottom

119 Filename before the warning and full path to it #361

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

119 Filename before the warning and full path to it #361

wants to merge 2 commits into from

Conversation

elenachavdarova
Copy link

Included full path to the file and moved the path in the start of the warning.

Fixes: #119

@elenachavdarova elenachavdarova requested a review from a team as a code owner April 7, 2023 12:01
@elenachavdarova elenachavdarova mentioned this pull request Apr 7, 2023
Open
danielbachhuber
danielbachhuber requested changes Apr 7, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber 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 the pull request, @elenachavdarova !

Can you update the existing functional tests so they pass?

Also, what do you think about using a relative path instead of the full path? I think a relative path (relative to the directory they're in) might be a little more developer-friendly.

@elenachavdarova
Copy link
Author

Hello @danielbachhuber,
Thank you for your reply. For the full path - my opinion is that relative is better as well, however on the issue this PR is for - #119 the request is for full path (especially when automating). This is why I have set this up this way.

As for the funcitonal tests - I checked the errors upon the failed tests and I am not sure how to include the full path (as it is a tmp one) to the expected output. I have updated the PR with file name moved to the start of the error.

@danielbachhuber
Copy link
Member

Thank you for your reply. For the full path - my opinion is that relative is better as well, however on the issue this PR is for - #119 the request is for full path (especially when automating). This is why I have set this up this way.

@elenachavdarova Sounds good!

As for the funcitonal tests - I checked the errors upon the failed tests and I am not sure how to include the full path (as it is a tmp one) to the expected output. I have updated the PR with file name moved to the start of the error.

This works for me.

I'll leave this to @swissspidy for a final review. He's AFK for the next week or so though, so it might be a little bit.

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

@danielbachhuber danielbachhuber danielbachhuber requested changes

@swissspidy swissspidy Awaiting requested review from swissspidy

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

File name BEFORE the Warning & full path
3 participants
@elenachavdarova @danielbachhuber @elenachavdarovasg