Skip to content

Code improvements and linting #640

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
Merged
merged 3 commits into from
Mar 14, 2018
Merged

Code improvements and linting #640

merged 3 commits into from
Mar 14, 2018

Conversation

OPpuolitaival
Copy link
Contributor

Small fixing like:

  • Initialise variable before usage
  • Match string arguments amount
  • Remove unused variables

@0xc0170 0xc0170 requested review from theotherjimmy and removed request for theotherjimmy March 13, 2018 13:39
theotherjimmy
theotherjimmy approved these changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

Looks like a good bit of linting.

@OPpuolitaival
Copy link
Contributor Author

@theotherjimmy I will fix that problem

screamerbg
screamerbg approved these changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@screamerbg screamerbg 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 PR. Other than the comment I left, everything else looks great!

mbed/mbed.py Outdated
@@ -1278,6 +1279,7 @@ def get_cache(self, url):
def set_cache(self, url):
up = urlparse(formaturl(url, 'https'))
if self.cache and up and up.netloc and os.path.isdir(self.path):
cpath = 'none'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not right. cpath = os.path.join(... should be moved before the try: instead

@OPpuolitaival
Copy link
Contributor Author

Fixed @screamerbg mentioned problem and added auto formatting

@screamerbg
Copy link
Contributor

screamerbg commented Mar 13, 2018 •
edited
Loading

@OPpuolitaival LGTM! Waiting for CI to finish.

@screamerbg
Copy link
Contributor

@OPpuolitaival Could you please rebase?

@OPpuolitaival
Copy link
Contributor Author

@screamerbg rebased

@screamerbg
Copy link
Contributor

@OPpuolitaival Can you remove the autoformatting? That's way too intrusive and I don't agree with some of the autoformatting rules.

@screamerbg
Copy link
Contributor

@OPpuolitaival I mean whatever was introduced in 85829a2

@OPpuolitaival
Copy link
Contributor Author

@screamerbg autoformatting removed. I did ask from Jimmy and he was okay with those and therefore added it in here

@screamerbg
Copy link
Contributor

@OPpuolitaival Thanks. Current change set is 15 changed lines vs 580 with autoformatter.

@OPpuolitaival
Copy link
Contributor Author

@screamerbg Sure, that is the idea of autoformatting that it does big job automatically

@screamerbg screamerbg merged commit 75900ee into ARMmbed:master Mar 14, 2018
@OPpuolitaival OPpuolitaival deleted the dev branch March 14, 2018 10:49
@screamerbg screamerbg changed the title Fixing code Code improvements and linting Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theotherjimmy theotherjimmy theotherjimmy approved these changes

@screamerbg screamerbg screamerbg approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@OPpuolitaival @screamerbg @theotherjimmy