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

fixed issue #2985 #3214

Merged
merged 11 commits into from
Mar 15, 2021
Merged

fixed issue #2985 #3214

merged 11 commits into from
Mar 15, 2021

Conversation

surajkumar-sk
Copy link
Contributor

@surajkumar-sk surajkumar-sk commented Mar 12, 2021

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

issue #2985 occurs in windows . replacing '\' with '/' fixes the error for windows but impling the changes to all platforms may result in errors so an if condition is added to replace slashes only in windows .

there was a ';' inside check.py which i believe was a syntax error so removed it .
Fixes # (issue)

#2985

Screenshots (optional)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project

  • I have performed a self-review of my own code

  • My changes generate no new warnings

  • New and existing unit tests pass locally with my changes

@sradhanjan
Copy link

@surajkumar-sk Thanks a lot, these changes worked for me.

@surajkumar-sk
Copy link
Contributor Author

@sradhanjan . I'm glad it worked . 🙂

if os.name == 'nt' :
self.config_path = self.config_path.replace('\\','/')
for j in range(0,len(self.source_files)):
self.source_files[j] = self.source_files[j].replace('\\','/')
Copy link
Contributor

Choose a reason for hiding this comment

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

A more Python way to do this would be:

self.source_files = [f.replace('\\', '/') for f in self.source_files]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a cool way . Concating for loop and statement into a single line . Now I'll always use that from now . Just a follow up , can we add more than one line of code in that format .

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean. This is called list-comprehension. This is just assigning to a new array it just created. The basic format is [val for f in list] where val can be any expression and is each element's value and list is the source list.

@@ -357,6 +357,12 @@ def lint(self, fix=False, force=False):
deps = self.source_files + [self.config_path]
if not force and not _must_build(self.output, deps):
return True
"""Windows shows an error when the file location has '\\' so for
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this isn't the function docstring. This should just be a comment. Also you don't need to explain what the code is doing, just why it is. So this would be better.

# Windows shows an error when the file location has '\\'.

"""Windows shows an error when the file location has '\\' so for
windows the below if condition changes the'\\' with '/'
in variable storing file locations """
if sys.platform == 'win32' :
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra space before :.

windows the below if condition changes the'\\' with '/'
in variable storing file locations """
if sys.platform == 'win32' :
self.config_path = self.config_path.replace('\\','/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Add space between arguments.

@shaka-bot
Copy link
Collaborator

Test Failure:

Traceback (most recent call last):
File "./build/compiler.py", line 363
SyntaxError: Non-ASCII character '\xe2' in file ./build/compiler.py on line 363, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details
Build step 'Execute shell' marked build as failure

@TheModMaker
Copy link
Contributor

Your code has a bunch of zero-width spaces in it (U+200B). Make sure your editor isn't adding these.

@surajkumar-sk
Copy link
Contributor Author

@TheModMaker Actually, I copy-pasted that if condition line which caused an encoding error. I removed that and retyped the condition. It should work on.

@surajkumar-sk
Copy link
Contributor Author

@TheModMaker Thank you for being so patient. I'll make sure to check for all the above mistakes in my next commit.

@shaka-bot
Copy link
Collaborator

All tests passed!

@TheModMaker TheModMaker merged commit 568321c into shaka-project:master Mar 15, 2021
joeyparrish pushed a commit that referenced this pull request Mar 17, 2021
joeyparrish pushed a commit that referenced this pull request Mar 17, 2021
joeyparrish pushed a commit that referenced this pull request Mar 17, 2021
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants