Closed Ravisaketi closed 2 years ago
CLA Assistant Lite bot All contributors have signed the CLA ✍️
I have read the CLA Document and I hereby sign the CLA
hi @c0c0n3 @chicco785 I have closed the previous pr #658 related to issue #656 and now I raise the new pr for issue #656 please check this pr #661 for the same issue
hi @Necravisaketi , good stuff, thanks so much for your contrib! If you could please rebase your PR on master since we've just merged #653
Hi, @c0c0n3 @chicco785 when I run the tests locally all tests were passed but in the CircleCi the test_others
tests are failing. Sometimes after running so many times, it will pass automatically.
@Necravisaketi the test_others
task fails b/c the geocoding test can't connect to redis it seems. not sure how that could possibly be related to your changes, but can you try figuring out what's causing that? I haven't seen this error on the other PRs we've merged, that's why I'm asking...
@Necravisaketi actually I'm wrong. If you look at #660 you'll see there's no code changes in there, but test_others
still failed. I've opened a separate issue about it: #666. Meanwhile we can ignore the failing test here, rebase on master, then merge. Thanks alot!
@Necravisaketi so we're going to ignore the test failure, but we still have a problem w/ the linter. I checked out your branch and ran the linter (lint.py.sh
) which failed w/ exit code of 2. Can you please do the same, see what files the linter reformats, then try to figure out what the issue is. Thanks alot!!
Hi, @c0c0n3 when I run the linter(lint.py.sh)
it shows requirement already satisfied.
and also I run lint.md.sh
it showed nothing. So can you please suggest any other way to solve this?
Hi @Necravisaketi thanks for checking this. I think we've bumped into another autopep version issue like in #618. The problem is that the autopep version the GitHub action uses is 1.5.4
whereas the one you have is 1.6.0
. Not your fault, it's actually our fault we should've pinned the autoped version in lint.py.sh
to be the same as the one used in the GitHub action.
Anyway, I've managed to reproduce why autopep 1.5.4
isn't happy about your code.
$ git clone https://github.com/Necravisaketi/ngsi-timeseries-api.git
$ cd ngsi-timeseries-api
$ git checkout pr#656
$ pipenv install
$ pipenv install autopep8==1.5.4
$ pipenv shell
$ autopep8 --exit-code --recursive --in-place --aggressive --aggressive .
$ echo $?
2
If I then diff the Python change set
$ git diff src/wq/ql/notify.py
I can see autopep added two newline breaks
diff --git a/src/wq/ql/notify.py b/src/wq/ql/notify.py
index 8dbd07b..c6ee561 100644
--- a/src/wq/ql/notify.py
+++ b/src/wq/ql/notify.py
@@ -10,10 +10,12 @@ import wq.core.cfg as cfg
from wq.ql.flaskutils import build_json_array_response_stream
import logging
+
def log():
logger = logging.getLogger(__name__)
return logger
+
class FiwareTaskId(CompositeTaskId):
def __init__(self,
Supposedly if you do the same, then the GitHub action should succeed...
@Necravisaketi thanks for making the linter happy---latest commit. The GitHub task still fails but I can't see why since if I run it locally it succeeds. So I'm going to merge this PR regardless. Thanks for your contrib, much appreciated!
Proposed changes
Added logs in src/wq/ql/notify.py fix for issue #656
Types of changes
What types of changes does your code introduce to the project? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...