Skip to content

Moved repository from Travis to GitHub Actions #49

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

Merged
merged 4 commits into from
Jan 12, 2020
Merged

Conversation

evaherrada
Copy link
Collaborator

No description provided.

@ladyada
Copy link
Member

ladyada commented Dec 28, 2019

failed to pass

@siddacious
Copy link
Contributor

@dherrada Let's wait on this one. I'll adjust the priority

@siddacious siddacious requested a review from tannewt January 7, 2020 01:24
@siddacious
Copy link
Contributor

@tannewt Can you take a look at the pylint failures? I'm wondering if they're caused by the actions change or if they were already there.

@siddacious
Copy link
Contributor

@dhalbert if you could take a look at the pylint failures and let Dylan or I know if they were something that the Actions switchover, perhaps because of a difference in how pylint is being called it would help move this PR towards resolution.

@siddacious
Copy link
Contributor

siddacious commented Jan 9, 2020

@dhalbert @tannewt nm, I figured it out; It looks like the failures were expected and numerous enough that you added the ignores to the pylint invocation rather than touching a bunch of files.

At least for now the actions setup doesn't allow for a per-repo override of the default pylint invocation, so I'll put in a separate PR to add the required ignores to the files themselves. Once we allow for overriding we can revert that patch.

@tannewt
Copy link
Member

tannewt commented Jan 9, 2020

@siddacious sounds good

@evaherrada
Copy link
Collaborator Author

@siddacious working on fixing merge conflicts now

@evaherrada
Copy link
Collaborator Author

@siddacious Ok. Looks like this is passing. After some googling and trying to fix this, I'm still not sure how to fix the merge conflict and since not many other people are online I've asked a friend of mine who is really good at git how I would go about fixing this, but it might take him a while to respond, so next time you're online, if I haven't fixed it by then, it'd be great to get some guidance on this.

@dhalbert
Copy link
Collaborator

@dherrada You can merge from upstream (and fix any merge conflicts). Do you have the upstream remote set? If not, do:

git remote add adafruit https://github.com/adafruit/Adafruit_CircuitPython_BLE
git remote -v    # check the remotes

Then do:

git fetch adafruit
git merge adafruit/master
# fix any merge conflicts with `git mergetool`
git push

@evaherrada
Copy link
Collaborator Author

@dhalbert thanks. I’ll try that

run: git describe --dirty --always --tags
- name: PyLint
run: |
pylint --disable=too-few-public-methods $( find . -path './adafruit*.py' )
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary, or at least that was the point of the other PR. Were you still getting failures without it?

Copy link
Collaborator Author

@evaherrada evaherrada Jan 12, 2020

Choose a reason for hiding this comment

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

@siddacious Yeah, because this branch still has the files without the disables in the classes themselves. After this gets merged, I'll make another PR removing that disable.

@evaherrada
Copy link
Collaborator Author

@siddacious A small correction. It was necessary before I did the merge (thanks @dhalbert) but now that I've done the merge, it isn't. Thanks for pointing that out.

@evaherrada evaherrada requested a review from siddacious January 12, 2020 22:45
@siddacious siddacious merged commit 310bd0e into master Jan 12, 2020
@dhalbert dhalbert deleted the dherrada-patch-1 branch January 27, 2020 22:39
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.

5 participants