ronantakizawa/codereview-bench huggingface dataset:
Repository: spotify/luigi
File: luigi/contrib/bigquery.py (Python)

Code before:

        if self.destination_format == 'CSV':
            # "Only exports to CSV may specify a field delimiter."
            job['configuration']['extract']['printHeader'] = self.print_header
            job['configuration']['extract']['fieldDelimiter'] = \
                self.field_delimiter

        bq_client.run_job(
            input.table.project_id,
            job,
            dataset=input.table.dataset)


# the original inconsistently capitalized aliases, for backwards compatibility
BigqueryClient = BigQueryClient
BigqueryTarget = BigQueryTarget
MixinBigqueryBulkComplete = MixinBigQueryBulkComplete
BigqueryLoadTask = BigQueryLoadTask
BigqueryRunQueryTask = BigQueryRunQueryTask
BigqueryCreateViewTask = BigQueryCreateViewTask
ExternalBigqueryTask = ExternalBigQueryTask


class BigQueryExecutionError(Exception):
    def __init__(self, job_id, error_message) -> None:
        super(BigQueryExecutionError, self).__init__('BigQuery job {} failed: {}'.format(job_id, error_message))
        self.error_message = error_message
        self.job_id = job_id


Review comment (refactor):
We can use Python3 syntax here to simplify because Python2 support is removed from Luigi.

```
super().__init__(...)
```

Code after:
                }
            }
        }

        if self.destination_format == 'CSV':
            # "Only exports to CSV may specify a field delimiter."
            job['configuration']['extract']['printHeader'] = self.print_header
            job['configuration']['extract']['fieldDelimiter'] = \
                self.field_delimiter

        bq_client.run_job(
            input.table.project_id,
            job,
            dataset=input.table.dataset)


# the original inconsistently capitalized aliases, for backwards compatibility
BigqueryClient = BigQueryClient
BigqueryTarget = BigQueryTarget
MixinBigqueryBulkComplete = MixinBigQueryBulkComplete
BigqueryLoadTask = BigQueryLoadTask
BigqueryRunQueryTask = BigQueryRunQueryTask
BigqueryCreateViewTask = BigQueryCreateViewTask
ExternalBigqueryTask = ExternalBigQueryTask


class BigQueryExecutionError(Exception):
    def __init__(self, job_id, error_message) -> None:
        """
        :param job_id: BigQuery Job ID
        :type job_id: str
        :param error_message: status['status']['errorResult'] for the failed job
        :type error_message: str
        """
        super().__init__('BigQuery job {} failed: {}'.format(job_id, error_message))
        self.error_message = error_message
        self.job_id = job_id
Repository: spotify/luigi
File: luigi/rpc.py (Python)

Code before:
HAS_UNIX_SOCKET = True
HAS_REQUESTS = True


try:
    import requests_unixsocket as requests
except ImportError:
    HAS_UNIX_SOCKET = False
    try:
        import requests
    except ImportError:
        HAS_REQUESTS = False


logger = logging.getLogger('luigi-interface')  # TODO: 'interface'?


def _urljoin(base, url):
    """
    Join relative URLs to base URLs like urllib.parse.urljoin but support
    arbitrary URIs (esp. 'http+unix://').
    """
    parsed = urlparse(base)
    scheme = parsed.scheme
    return urlparse(
        urljoin(parsed._replace(scheme='http').geturl(), url if parsed.path == '' else parsed.path + url)
    )._replace(scheme=scheme).geturl()


class RPCError(Exception):

    def __init__(self, message, sub_exception=None):
        super(RPCError, self).__init__(message)
        self.sub_exception = sub_exception


class _FetcherInterface(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def fetch(self, full_url, body, timeout):
        pass

    @abc.abstractmethod
    def close(self):
        pass


class URLLibFetcher(_FetcherInterface):
    raises = (URLError, socket.timeout)

    def _create_request(self, full_url, body=None):
        # when full_url contains basic auth info, extract it and set the Authorization header

Review comment (suggestion):
I'm not sure i understand the end result impact here.

I also don't understand why you do `parsed.path + url` instead of `url + parsed.path`.

It would be preferred if you could provide a unittest in https://github.com/spotify/luigi/blob/master/test/rpc_test.py to show this behavior.

Code after:
HAS_UNIX_SOCKET = True
HAS_REQUESTS = True


try:
    import requests_unixsocket as requests
except ImportError:
    HAS_UNIX_SOCKET = False
    try:
        import requests
    except ImportError:
        HAS_REQUESTS = False


logger = logging.getLogger('luigi-interface')  # TODO: 'interface'?


def _urljoin(base, url):
    """
    Join relative URLs to base URLs like urllib.parse.urljoin but support
    arbitrary URIs (esp. 'http+unix://').
    base part is fixed or mounted point, every url contains full base part.
    """
    parsed = urlparse(base)
    scheme = parsed.scheme
    return urlparse(
        urljoin(parsed._replace(scheme='http').geturl(), parsed.path + (url if url[0] == '/' else '/' + url))
    )._replace(scheme=scheme).geturl()


class RPCError(Exception):

    def __init__(self, message, sub_exception=None):
        super(RPCError, self).__init__(message)
        self.sub_exception = sub_exception


class _FetcherInterface(metaclass=abc.ABCMeta):
    @abc.abstractmethod
    def fetch(self, full_url, body, timeout):
        pass

    @abc.abstractmethod
    def close(self):
        pass


class URLLibFetcher(_FetcherInterface):
    raises = (URLError, socket.timeout)

    def _create_request(self, full_url, body=None):
Repository: spotify/luigi
File: test/contrib/bigquery_test.py (Python)

Code before:
                return BigQueryTarget(project_id='proj', dataset_id='ds', table_id='t')

            def output(self):
                return GCSTarget('gs://_')

        job = MyBigQueryExtractTask()
        job.run()

        expected_body = {
            'configuration': {
                'extract': {
                    'sourceTable': {'projectId': 'proj', 'datasetId': 'ds', 'tableId': 't'},
                    'destinationUris': ['gs://_'],
                    'destinationFormat': 'AVRO',
                    'compression': 'NONE',
                    'useAvroLogicalTypes': True
                }
            }
        }
        run_job.assert_called_with('proj', expected_body, dataset=BQDataset('proj', 'ds', None))


class BigQueryClientTest(unittest.TestCase):

    def test_retry_succeeds_on_second_attempt(self):
        client = MagicMock(spec=bigquery.BigQueryClient)
        attempts = [0]

        @bigquery.bq_retry
        def fail_once(bq_client):
            attempts[0] += 1
            if attempts[0] == 1:
                raise errors.HttpError(
                    resp=MagicMock(status=500),
                    content=b'{"error": {"message": "stub"}',
                )
            else:
                return MagicMock(status=200)

        response = fail_once(client)

        self.assertEqual(attempts[0], 2)
        self.assertEqual(response.status, 200)


Review comment (suggestion):
I think we want to assert `client. _initialise_client` is invoked due to retry.

Code after:

        job = MyBigQueryExtractTask()
        job.run()

        expected_body = {
            'configuration': {
                'extract': {
                    'sourceTable': {'projectId': 'proj', 'datasetId': 'ds', 'tableId': 't'},
                    'destinationUris': ['gs://_'],
                    'destinationFormat': 'AVRO',
                    'compression': 'NONE',
                    'useAvroLogicalTypes': True
                }
            }
        }
        run_job.assert_called_with('proj', expected_body, dataset=BQDataset('proj', 'ds', None))


class BigQueryClientTest(unittest.TestCase):

    def test_retry_succeeds_on_second_attempt(self):
        try:
            from googleapiclient import errors
        except ImportError:
            raise unittest.SkipTest('Unable to load googleapiclient module')
        client = MagicMock(spec=BigQueryClient)
        attempts = 0

        @bigquery.bq_retry
        def fail_once(bq_client):
            nonlocal attempts
            attempts += 1
            if attempts == 1:
                raise errors.HttpError(
                    resp=MagicMock(status=500),
                    content=b'{"error": {"message": "stub"}',
                )
            else:
                return MagicMock(status=200)

        response = fail_once(client)
        client._initialise_client.assert_called_once()
        self.assertEqual(attempts, 2)
        self.assertEqual(response.status, 200)
Repository: spotify/luigi
File: luigi/contrib/mongodb.py (Python)

Code before:
# See the License for the specific language governing permissions and
# limitations under the License.
#

from luigi.target import Target


class MongoTarget(Target):

    """ Target for a resource in MongoDB """

    def __init__(self, mongo_client, index, collection):
        """
        :param mongo_client: MongoClient instance
        :type mongo_client: MongoClient
        :param index: database index
        :type index: str
        :param collection: index collection
        :type collection: str
        """
        self._mongo_client = mongo_client
        self._index = index
        self._collection = collection

    def __str__(self):
        return '%s/%s' % (self._index, self._collection)

    def get_collection(self):
        """
        Return targeted mongo collection to query on
        """
        db_mongo = self._mongo_client[self._index]
        return db_mongo[self._collection]

    def get_index(self):
        """
        Return targeted mongo index to query on
        """
        return self._mongo_client[self._index]


class MongoCellTarget(MongoTarget):

    """ Target for a ressource in a specific field from a MongoDB document """

    def __init__(self, mongo_client, index, collection, document_id, path):
        """
        :param document_id: targeted mongo document
        :type document_id: str
        :param path: full path to the targeted field in the mongo document
        :type path: str

Review comment (suggestion):
personally, i'd prefer the use of `str.format()` or f-strings

Code after:
# See the License for the specific language governing permissions and
# limitations under the License.
#

from luigi.target import Target


class MongoTarget(Target):

    """ Target for a resource in MongoDB """

    def __init__(self, mongo_client, index, collection):
        """
        :param mongo_client: MongoClient instance
        :type mongo_client: MongoClient
        :param index: database index
        :type index: str
        :param collection: index collection
        :type collection: str
        """
        self._mongo_client = mongo_client
        self._index = index
        self._collection = collection

    def __str__(self):
        return f'{self._index}/{self._collection}'

    def get_collection(self):
        """
        Return targeted mongo collection to query on
        """
        db_mongo = self._mongo_client[self._index]
        return db_mongo[self._collection]

    def get_index(self):
        """
        Return targeted mongo index to query on
        """
        return self._mongo_client[self._index]


class MongoCellTarget(MongoTarget):

    """ Target for a ressource in a specific field from a MongoDB document """

    def __init__(self, mongo_client, index, collection, document_id, path):
        """
        :param document_id: targeted mongo document
        :type document_id: str
        :param path: full path to the targeted field in the mongo document
        :type path: str
Repository: square/leakcanary
File: leakcanary-android/src/main/java/com/squareup/leakcanary/internal/LeakCanarySingleThreadFactory.java (Java)

Code before:
package com.squareup.leakcanary.internal;

import java.util.concurrent.ThreadFactory;

/**
 * Copied from {@link java.util.concurrent.Executors#defaultThreadFactory()}. This is intended to
 * only be used with a single thread executor.
 */
final class LeakCanarySingleThreadFactory implements ThreadFactory {

  private final ThreadGroup group;
  private final String threadName;

  LeakCanarySingleThreadFactory(String name) {
    SecurityManager s = System.getSecurityManager();
    group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();
    threadName = "LeakCanary-" + name;
  }

  @Override public Thread newThread(Runnable r) {
    Thread t = new Thread(group, r, threadName, 0);
    if (t.isDaemon()) {
      t.setDaemon(false);
    }
    if (t.getPriority() != Thread.NORM_PRIORITY) {
      t.setPriority(Thread.NORM_PRIORITY);
    }
    return t;
  }
}


Review comment (security):
This does way more than what we need (ie we won't need to deal with thread groups or change `daemon` or `priority` or access the security manager).

A simpler implementation would do well:

``` java
final class LeakCanaryThreadFactory implements ThreadFactory {
private final String threadName;

  LeakCanaryThreadFactory(String threadName) {
    this.threadName = "LeakCanary-" + threadName;
  }

  @Override public Thread newThread(Runnable runnable) {
    return new Thread(runnable, threadName);
  }
}
```


Code after:
/*
 * Copyright (C) 2015 Square, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package com.squareup.leakcanary.internal;

import java.util.concurrent.ThreadFactory;

/**
 * This is intended to only be used with a single thread executor.
 */
final class LeakCanarySingleThreadFactory implements ThreadFactory {

  private final String threadName;

  LeakCanarySingleThreadFactory(String threadName) {
    this.threadName = "LeakCanary-" + threadName;
  }

  @Override public Thread newThread(Runnable runnable) {
    return new Thread(runnable, threadName);
  }
}
Repository: square/leakcanary
File: leakcanary-android/src/main/java/com/squareup/leakcanary/internal/LeakCanarySingleThreadFactory.java (Java)

Code before:
package com.squareup.leakcanary.internal;

import java.util.concurrent.ThreadFactory;

/**
 * Copied from {@link java.util.concurrent.Executors#defaultThreadFactory()}. This is intended to
 * only be used with a single thread executor.
 */
final class LeakCanarySingleThreadFactory implements ThreadFactory {

  private final ThreadGroup group;
  private final String threadName;

  LeakCanarySingleThreadFactory(String name) {
    SecurityManager s = System.getSecurityManager();
    group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();
    threadName = "LeakCanary-" + name;
  }

  @Override public Thread newThread(Runnable r) {
    Thread t = new Thread(group, r, threadName, 0);
    if (t.isDaemon()) {
      t.setDaemon(false);
    }
    if (t.getPriority() != Thread.NORM_PRIORITY) {
      t.setPriority(Thread.NORM_PRIORITY);
    }
    return t;
  }
}


Review comment (suggestion):
I'm OK with this. I only pulled in this implementation to keep it as close to what is currently being used as possible.


Code after:
/*
 * Copyright (C) 2015 Square, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package com.squareup.leakcanary.internal;

import java.util.concurrent.ThreadFactory;

/**
 * This is intended to only be used with a single thread executor.
 */
final class LeakCanarySingleThreadFactory implements ThreadFactory {

  private final String threadName;

  LeakCanarySingleThreadFactory(String threadName) {
    this.threadName = "LeakCanary-" + threadName;
  }

  @Override public Thread newThread(Runnable runnable) {
    return new Thread(runnable, threadName);
  }
}
Repository: square/leakcanary
File: leakcanary-android/src/main/java/com/squareup/leakcanary/LeakCanary.java (Java)

Code before:
        + LIBRARY_VERSION
        + " "
        + GIT_SHA
        + "\n"
        + "* Durations: watch="
        + heapDump.watchDurationMs
        + "ms, gc="
        + heapDump.gcDurationMs
        + "ms, heap dump="
        + heapDump.heapDumpDurationMs
        + "ms, analysis="
        + result.analysisDurationMs
        + "ms"
        + "\n"
        + detailedString;

    return info;
  }

  /**
   * Whether the current process is the process running the {@link HeapAnalyzerService}, which is
   * a different process than the normal app process.
   */
  public static boolean isInAnalyzerProcess(Context context) {
    // This only needs to be computed once per process.
    if (isInAnalyzerProcess == null) {
      isInAnalyzerProcess = isInServiceProcess(context, HeapAnalyzerService.class);
    }
    return isInAnalyzerProcess;
  }

  private LeakCanary() {
    throw new AssertionError();
  }
}


Review comment (suggestion):
If you read to a local you avoid having to double-read the volatile. You have to update the local and static inside the `if` then.

Code after:
        + " "
        + GIT_SHA
        + "\n"
        + "* Durations: watch="
        + heapDump.watchDurationMs
        + "ms, gc="
        + heapDump.gcDurationMs
        + "ms, heap dump="
        + heapDump.heapDumpDurationMs
        + "ms, analysis="
        + result.analysisDurationMs
        + "ms"
        + "\n"
        + detailedString;

    return info;
  }

  /**
   * Whether the current process is the process running the {@link HeapAnalyzerService}, which is
   * a different process than the normal app process.
   */
  public static boolean isInAnalyzerProcess(Context context) {
    Boolean isInAnalyzerProcess = LeakCanaryInternals.isInAnalyzerProcess;
    // This only needs to be computed once per process.
    if (isInAnalyzerProcess == null) {
      isInAnalyzerProcess = isInServiceProcess(context, HeapAnalyzerService.class);
      LeakCanaryInternals.isInAnalyzerProcess = isInAnalyzerProcess;
    }
    return isInAnalyzerProcess;
  }

  private LeakCanary() {
    throw new AssertionError();
  }
}
Repository: square/leakcanary
File: leakcanary-sample/src/androidTest/java/com/example/leakcanary/tests/TuPeuxPasTest.kt (Kotlin)

Code before:

/**
 * This UI test looks like it should succeed, but it will actually fail because
 * it triggers a leak.
 *
 * Run this test with:
 *
 * ./gradlew leakcanary-sample:connectedCheck
 *
 * To set this up, we installed a special RefWatcher dedicated to detecting leaks in
 * instrumentation tests in [InstrumentationExampleApplication], and then added the FailTestOnLeakRunListener
 * to the config of our build.gradle:
 *
 * testInstrumentationRunnerArgument "listener", "com.squareup.leakcanary.FailTestOnLeakRunListener"
 *
 *
 * Why is this class named "TuPeuxPasTest"?
 *
 * This test fails, intentionally. In French, "Tu peux pas test" could mean "you cannot test"
 * written with poor grammar. Except, that's not what it means.
 * If you're curious, interested in French and have time to waste:
 * https://www.youtube.com/watch?v=DZZpbmAc-0A
 * https://www.youtube.com/watch?v=nHeAA6X-XUQ
 */
class TuPeuxPasTest {

  @Rule
  var activityRule = ActivityTestRule(MainActivity::class.java)

  @Test
  fun clickAsyncWork() {
    onView(withId(R.id.async_work)).perform(click())
  }

  @Test
  fun asyncButtonHasStartText() {
    onView(withId(R.id.async_work)).check(matches(withText(R.string.start_async_work)))
  }
}


Review comment (suggestion):
We should be able to use just `@Rule` if we add the `@JvmStatic` annotation

Code after:
/**
 * This UI test looks like it should succeed, but it will actually fail because
 * it triggers a leak.
 *
 * Run this test with:
 *
 * ./gradlew leakcanary-sample:connectedCheck
 *
 * To set this up, we installed a special RefWatcher dedicated to detecting leaks in
 * instrumentation tests in [InstrumentationExampleApplication], and then added the FailTestOnLeakRunListener
 * to the config of our build.gradle:
 *
 * testInstrumentationRunnerArgument "listener", "com.squareup.leakcanary.FailTestOnLeakRunListener"
 *
 *
 * Why is this class named "TuPeuxPasTest"?
 *
 * This test fails, intentionally. In French, "Tu peux pas test" could mean "you cannot test"
 * written with poor grammar. Except, that's not what it means.
 * If you're curious, interested in French and have time to waste:
 * https://www.youtube.com/watch?v=DZZpbmAc-0A
 * https://www.youtube.com/watch?v=nHeAA6X-XUQ
 */
class TuPeuxPasTest {

  @get:Rule
  var activityRule = ActivityTestRule(MainActivity::class.java)

  @Test
  fun clickAsyncWork() {
    onView(withId(R.id.async_work)).perform(click())
  }

  @Test
  fun asyncButtonHasStartText() {
    onView(withId(R.id.async_work)).check(matches(withText(R.string.start_async_work)))
  }
}
Repository: square/leakcanary
File: leakcanary-android-instrumentation/src/main/java/leakcanary/FailAnnotatedTestOnLeakRunListener.kt (Kotlin)

Code before:
package leakcanary

import org.junit.runner.Description

class FailAnnotatedTestOnLeakRunListener: FailTestOnLeakRunListener() {

  override fun skipLeakDetectionReason(description: Description): String? {
    return if (description.getAnnotation(FailTestOnLeak::class.java) != null)
          null
      else
          "skipped leak detection"
  }
}

Review comment (bug):
Can you add javadoc and mention `@FailTestOnLeak` ?

Code after:
package leakcanary

import org.junit.runner.Description
import org.junit.runner.notification.RunListener

/**
 *
 * A JUnit [RunListener] subclassing [FailTestOnLeakRunListener] for detecting memory
 * leaks in Android instrumentation tests only when the [FailTestOnLeak] annotation
 * is used.
 *
 * @see FailTestOnLeak
 */
class FailAnnotatedTestOnLeakRunListener: FailTestOnLeakRunListener() {

  override fun skipLeakDetectionReason(description: Description): String? {
    return if (description.getAnnotation(FailTestOnLeak::class.java) != null)
          null
      else
          "test is not annotated with @FailTestOnLeak"
  }
}
Repository: square/leakcanary
File: leakcanary-android-instrumentation/src/main/java/leakcanary/FailAnnotatedTestOnLeakRunListener.kt (Kotlin)

Code before:
package leakcanary

import org.junit.runner.Description

class FailAnnotatedTestOnLeakRunListener: FailTestOnLeakRunListener() {

  override fun skipLeakDetectionReason(description: Description): String? {
    return if (description.getAnnotation(FailTestOnLeak::class.java) != null)
          null
      else
          "skipped leak detection"
  }
}

Review comment (bug):
How about `"skipped leak detection"` => `"test is not annotated with @FailTestOnLeak"` ?

Code after:
package leakcanary

import org.junit.runner.Description
import org.junit.runner.notification.RunListener

/**
 *
 * A JUnit [RunListener] subclassing [FailTestOnLeakRunListener] for detecting memory
 * leaks in Android instrumentation tests only when the [FailTestOnLeak] annotation
 * is used.
 *
 * @see FailTestOnLeak
 */
class FailAnnotatedTestOnLeakRunListener: FailTestOnLeakRunListener() {

  override fun skipLeakDetectionReason(description: Description): String? {
    return if (description.getAnnotation(FailTestOnLeak::class.java) != null)
          null
      else
          "test is not annotated with @FailTestOnLeak"
  }
}