ronantakizawa/codereview-bench huggingface dataset:
Repository: wagtail/wagtail
File: wagtail/users/utils.py (Python)

Code before:


def get_gravatar_url(email, size=50, default_params={}):
    DEFAULT_IMAGE = "mp"
    DEFAULT_GRAVATAR_PROVIDER_URL = "www.gravatar.com/avatar"

    params = {
        "s": int(size)
        * 2,  # requested at retina size by default and scaled down at point of use with css
        "d": DEFAULT_IMAGE,
    }

    gravatar_provider_url = getattr(
        settings, "WAGTAIL_GRAVATAR_PROVIDER_URL", DEFAULT_GRAVATAR_PROVIDER_URL
    )

    if (not email) or (gravatar_provider_url is None):
        return None

    parsed_url = urlparse(gravatar_provider_url.rstrip("/"))

    if parsed_url.query:
        params.update(
            **{key: value[-1] for key, value in parse_qs(parsed_url.query).items()},
        )
    params.update(**default_params)

    email_hash = safe_md5(
        email.lower().encode("utf-8"), usedforsecurity=False
    ).hexdigest()

    gravatar_url = (
        f"//{parsed_url.netloc}{parsed_url.path}/{email_hash}?{urlencode(params)}"
    )

    return gravatar_url


def get_deleted_user_display_name(user_id):
    # Use a string placeholder as the user id could be non-numeric
    return _("user %(id)s (deleted)") % {"id": user_id}


Review comment (refactor):
Then, move this to up above, setting up the base params first, then allowing the params from the url to override this.

```py
    params = {
        **default_params
        # requested at retina size by default and scaled down at point of use with css
        "s": int(size) * 2,
    }
```


Code after:


def get_gravatar_url(email, size=50, default_params={"d": "mp"}):
    """
    See https://gravatar.com/site/implement/images/ for Gravatar image options.

    Example usage:

    .. code-block:: python

        # Basic usage
        gravatar_url = get_gravatar_url('user@example.com')

        # Customize size and default image
        gravatar_url = get_gravatar_url(
            'user@example.com',
            size=100,
            default_params={'d': 'robohash', 'f': 'y'}
        )

    Note:
        If any parameter in ``default_params`` also exists in the provider URL,
        it will be overridden by the provider URL's query parameter.
    """

    gravatar_provider_url = getattr(
        settings, "WAGTAIL_GRAVATAR_PROVIDER_URL", "//www.gravatar.com/avatar"
    )

    if (not email) or (gravatar_provider_url is None):
        return None

    parsed_url = urlparse(gravatar_provider_url)

    params = {
        **default_params,
        **(parse_qs(parsed_url.query or "")),
        # requested at retina size by default and scaled down at point of use with css
        "s": int(size) * 2,
    }

    email_hash = safe_md5(
        email.lower().encode("utf-8"), usedforsecurity=False
    ).hexdigest()

    parsed_url = parsed_url._replace(
        path=f"{parsed_url.path.rstrip('/')}/{email_hash}",
        query=urlencode(params, doseq=True),
    )

    gravatar_url = urlunparse(parsed_url)
Repository: wagtail/wagtail
File: wagtail/users/utils.py (Python)

Code before:

    return True


def get_gravatar_url(email, size=50, default_params={}):
    DEFAULT_IMAGE = "mp"
    DEFAULT_GRAVATAR_PROVIDER_URL = "www.gravatar.com/avatar"

    params = {
        "s": int(size)
        * 2,  # requested at retina size by default and scaled down at point of use with css
        "d": DEFAULT_IMAGE,
    }

    gravatar_provider_url = getattr(
        settings, "WAGTAIL_GRAVATAR_PROVIDER_URL", DEFAULT_GRAVATAR_PROVIDER_URL
    )

    if (not email) or (gravatar_provider_url is None):
        return None

    parsed_url = urlparse(gravatar_provider_url.rstrip("/"))

    if parsed_url.query:
        params.update(
            **{key: value[-1] for key, value in parse_qs(parsed_url.query).items()},
        )
    params.update(**default_params)

    email_hash = safe_md5(
        email.lower().encode("utf-8"), usedforsecurity=False
    ).hexdigest()

    gravatar_url = (
        f"//{parsed_url.netloc}{parsed_url.path}/{email_hash}?{urlencode(params)}"
    )

    return gravatar_url


def get_deleted_user_display_name(user_id):
    # Use a string placeholder as the user id could be non-numeric
    return _("user %(id)s (deleted)") % {"id": user_id}


Review comment (suggestion):
You probably do not need to do this array handling if you use `doseq=True`.

```suggestion
            **parse_qs(parsed_url.query),
```

Code after:

    return True


def get_gravatar_url(email, size=50, default_params={"d": "mp"}):
    """
    See https://gravatar.com/site/implement/images/ for Gravatar image options.

    Example usage:

    .. code-block:: python

        # Basic usage
        gravatar_url = get_gravatar_url('user@example.com')

        # Customize size and default image
        gravatar_url = get_gravatar_url(
            'user@example.com',
            size=100,
            default_params={'d': 'robohash', 'f': 'y'}
        )

    Note:
        If any parameter in ``default_params`` also exists in the provider URL,
        it will be overridden by the provider URL's query parameter.
    """

    gravatar_provider_url = getattr(
        settings, "WAGTAIL_GRAVATAR_PROVIDER_URL", "//www.gravatar.com/avatar"
    )

    if (not email) or (gravatar_provider_url is None):
        return None

    parsed_url = urlparse(gravatar_provider_url)

    params = {
        **default_params,
        **(parse_qs(parsed_url.query or "")),
        # requested at retina size by default and scaled down at point of use with css
        "s": int(size) * 2,
    }

    email_hash = safe_md5(
        email.lower().encode("utf-8"), usedforsecurity=False
    ).hexdigest()

    parsed_url = parsed_url._replace(
        path=f"{parsed_url.path.rstrip('/')}/{email_hash}",
        query=urlencode(params, doseq=True),
    )
Repository: wagtail/wagtail
File: wagtail/users/tests/test_utils.py (Python)

Code before:
from django.test import TestCase, override_settings

from wagtail.users.utils import get_gravatar_url


class TestGravatar(TestCase):
    def test_gravatar_default(self):
        # Test with the default settings
        self.assertEqual(
            get_gravatar_url("something@example.com"),
            "//www.gravatar.com/avatar/76ebd6fecabc982c205dd056e8f0415a?d=mp&s=100",
        )

    def test_gravatar_custom_size(self):
        # Test with a custom size (note that the size will be doubled)
        self.assertEqual(
            get_gravatar_url("something@example.com", size=100),
            "//www.gravatar.com/avatar/76ebd6fecabc982c205dd056e8f0415a?d=mp&s=200",
        )

    @override_settings(
        WAGTAIL_GRAVATAR_PROVIDER_URL="https://robohash.org/avatar?d=robohash&s=200"
    )
    def test_gravatar_params_that_overlap(self):
        # Test with params that overlap with default s (size) and d (default_image)
        # Also test the size is not overridden by the provider URL's query parameters.
        self.assertEqual(
            get_gravatar_url("something@example.com", size=80),
            "https://robohash.org/avatar/76ebd6fecabc982c205dd056e8f0415a?d=robohash&s=160",
        )

    @override_settings(WAGTAIL_GRAVATAR_PROVIDER_URL="https://robohash.org/avatar?f=y")
    def test_gravatar_params_that_dont_overlap(self):

Review comment (suggestion):
```suggestion
        """ Test with the default settings. """
```

This is not critical but it might be nice to use the triple quotations for test docs, it makes it a bit easier to read and while we do not use this consistently in the unit tests it's nice to write new tests with this approach.

Code after:
from django.test import TestCase, override_settings

from wagtail.users.utils import get_gravatar_url


class TestGravatar(TestCase):
    def test_gravatar_default(self):
        """Test with the default settings"""
        self.assertEqual(
            get_gravatar_url("something@example.com"),
            "//www.gravatar.com/avatar/76ebd6fecabc982c205dd056e8f0415a?d=mp&s=100",
        )

    def test_gravatar_custom_size(self):
        """Test with a custom size (note that the size will be doubled)"""
        self.assertEqual(
            get_gravatar_url("something@example.com", size=100),
            "//www.gravatar.com/avatar/76ebd6fecabc982c205dd056e8f0415a?d=mp&s=200",
        )

    @override_settings(
        WAGTAIL_GRAVATAR_PROVIDER_URL="https://robohash.org/avatar?d=robohash&s=200"
    )
    def test_gravatar_params_that_overlap(self):
        """
        Test with params that overlap with default s (size) and d (default_image)
        Also test the `s` is not overridden by the provider URL's query parameters.
        """
        self.assertEqual(
            get_gravatar_url("something@example.com", size=80),
            "https://robohash.org/avatar/76ebd6fecabc982c205dd056e8f0415a?d=robohash&s=160",
        )
Repository: wagtail/wagtail
File: client/src/controllers/RevealController.ts (TypeScript)

Code before:
    const storeValue = this.storeValue;
    const key = `wagtail:${this.identifier}:opened`;
    if (storeValue) {
      try {
        const storedValue = localStorage.getItem(key);
        if (storedValue === storeValue) {
          return true;
        }
      } catch (error) {
        // Ignore if localStorage is not available
      }
    }
    return false;
  }

  set stored(isOpened) {
    const storeValue = this.storeValue;
    const key = `wagtail:${this.identifier}:opened`;

    try {
      if (isOpened) {
        localStorage.setItem(key, storeValue);
      } else {
        localStorage.removeItem(key);
      }
    } catch (error) {
      // Ignore if localStorage is not available
    }
  }

  disconnect() {
    this.cleanUpPeekListener?.call(this);
  }
}


Review comment (refactor):
Maybe we should abstract this out as a getter. 

```
get localStorageKey() {
  return `wagtail:${this.identifier}:opened`;
}
```


Then. We can use it like. 

```
const key = this.localStorageKey
```


Code after:
  }

  /**
   * Collects all toggles, those controlled by this controller and any external
   * that have an aria-controls references to any content target elements.
   */
  get toggles() {
    const toggles = this.contentTargets
      .map((content) => content.id)
      .flatMap((id) =>
        Array.from(
          document.querySelectorAll<HTMLButtonElement>(
            `[aria-controls~="${id}"]`,
          ),
        ),
      )
      .concat(...this.toggleTargets);
    return Array.from(new Set(toggles));
  }

  /**
   * Set the inner icon on any toggles (in scope or out of scope).
   */
  updateToggleIcon(isOpenIcon = false) {
    if (!this.hasCloseIconClass || !this.hasOpenIconClass) return;
    const closeIconClass = this.closeIconClass;
    const openIconClass = this.openIconClass;
    if (closeIconClass === openIconClass) return;

    this.toggles
      .map((toggle) => {
        const iconElement = toggle.querySelector<HTMLSpanElement>('.icon');
        const useElement = iconElement?.querySelector<SVGUseElement>('use');
        if (!useElement || !iconElement) return [];
        return [iconElement, useElement] as const;
      })
      .filter(({ length }) => length)
      .forEach(([iconElement, useElement]) => {
        if (isOpenIcon) {
          iconElement.classList.remove(closeIconClass);
          iconElement.classList.add(openIconClass);
          useElement.setAttribute('href', `#${openIconClass}`);
        } else {
          iconElement.classList.remove(openIconClass);
          iconElement.classList.add(closeIconClass);
          useElement.setAttribute('href', `#${closeIconClass}`);
        }
      });
  }

  /**
Repository: wagtail/wagtail
File: client/src/controllers/RevealController.ts (TypeScript)

Code before:
    this.closedValue = !this.closedValue;
  }

  /**
   * Collects all toggles, those controlled by this controller and any external
   * that have an aria-controls references to any content target elements.
   */
  get toggles() {
    const toggles = this.contentTargets
      .map((content) => content.id)
      .flatMap((id) =>
        Array.from(
          document.querySelectorAll<HTMLButtonElement>(
            `[aria-controls~="${id}"]`,
          ),
        ),
      )
      .concat(...this.toggleTargets);
    return Array.from(new Set(toggles));
  }

  /**
   * Set the inner icon on any toggles (in scope or out of scope).
   */
  updateToggleIcon(isOpenIcon = false) {
    if (!this.hasCloseIconClass || !this.hasOpenIconClass) return;
    const closeIconClass = this.closeIconClass;
    const openIconClass = this.openIconClass;
    if (closeIconClass === openIconClass) return;

    this.toggles
      .map((toggle) => {
        const iconElement = toggle.querySelector<HTMLSpanElement>('.icon');
        const useElement = iconElement?.querySelector<SVGUseElement>('use');
        if (!useElement || !iconElement) return [];
        return [iconElement, useElement] as const;
      })
      .filter(({ length }) => length)
      .forEach(([iconElement, useElement]) => {
        if (isOpenIcon) {
          iconElement.classList.remove(closeIconClass);
          iconElement.classList.add(openIconClass);
          useElement.setAttribute('href', `#${openIconClass}`);
        } else {
          iconElement.classList.remove(openIconClass);
          iconElement.classList.add(closeIconClass);
          useElement.setAttribute('href', `#${closeIconClass}`);
        }
      });
  }


Review comment (suggestion):
Let's leave the code a bit cleaner, adding JSdoc items along the way.

```suggestion
  /**
   * Open (show) the reveal content.
   */
  open() {
```

Code after:
    }

    if (isInitial) return;
    // If we have known toggles, dispatch on those buttons
    toggles.forEach((target) => {
      this.dispatch('toggled', {
        cancelable: false,
        detail: {
          closed: shouldClose,
        },
        target,
      });
    });
  }

  /**
   * Close (hide) the reveal content.
   */
  close() {
    this.closedValue = true;
  }

  /**
   * Open (show) the reveal content.
   */
  open() {
    this.closedValue = false;
  }

  peek() {
    if (this.closedValue) {
      this.peekingValue = true;
      this.open();
    }
  }

  /**
   * Toggle the open/closed state of the controller, accounting for the peeking state (visually open, but not 'fixed' as open).
   * The updated closed value will be stored in local storage if available.
   */
  toggle() {
    this.stored = this.peekingValue ? false : !this.closedValue;

    if (this.peekingValue) {
      this.peekingValue = false;
      // if peeking and toggle clicked, is already open
      // visually set the icon to the opened variant so it can be closed.
      this.updateToggleIcon(false);
      return;
    }
    this.closedValue = !this.closedValue;
Repository: wagtail/wagtail
File: client/src/controllers/RevealController.ts (TypeScript)

Code before:
   */
  get localStorageKey() {
    const storeValue = this.storageKeyValue;
    if (!storeValue) return null;
    return ['wagtail', this.identifier, storeValue].join(':');
  }

  /**
   * Get the stored (closed) state of this controller from local storage.
   * If the key is not available, return undefined.
   */
  get stored(): boolean | undefined {
    const key = this.localStorageKey;
    if (!key) return undefined;
    try {
      const value = localStorage.getItem(key);
      if (value === null) return undefined;
      return value === 'closed';
    } catch (error) {
      //  Ignore if localStorage is not available.
    }
    return undefined;
  }

  /**
   * Set the stored state (if closed) of this controller in the local storage.
   * If the store key value is not set, do nothing.
   */
  set stored(isClosed: boolean) {
    const key = this.localStorageKey;
    if (!key) return;
    try {
      if (isClosed) {
        localStorage.setItem(key, 'closed');
      } else {
        localStorage.setItem(key, 'opened');
      }
    } catch (error) {
      // Ignore if localStorage is not available
    }
  }

  disconnect() {
    this.cleanUpPeekListener?.call(this);
  }
}


Review comment (suggestion):
```suggestion
   * Set the stored state of this controller in the local storage.
```

We are no longer just storing if closed.

Code after:
        }
      });
  }

  /**
   * Prepare a unique local storage key for this controller joined with the store value,
   * if not provided then assume we do not want to store the state of this controller.
   */
  get localStorageKey() {
    const storeValue = this.storageKeyValue;
    if (!storeValue) return null;
    return ['wagtail', this.identifier, storeValue].join(':');
  }

  /**
   * Get the stored (closed) state of this controller from local storage.
   * If the key is not available, return undefined.
   */
  get stored(): boolean | undefined {
    const key = this.localStorageKey;
    if (!key) return undefined;
    try {
      const value = localStorage.getItem(key);
      if (value === null) return undefined;
      return value === RevealState.CLOSED;
    } catch (error) {
      //  Ignore if localStorage is not available.
    }
    return undefined;
  }

  /**
   * Set the stored state of this controller in the local storage.
   * If the store key value is not set, do nothing.
   */
  set stored(isClosed: boolean) {
    const key = this.localStorageKey;
    if (!key) return;
    try {
      if (isClosed) {
        localStorage.setItem(key, RevealState.CLOSED);
      } else {
        localStorage.setItem(key, RevealState.OPENED);
      }
    } catch (error) {
      // Ignore if localStorage is not available
    }
  }

  disconnect() {
    this.cleanUpPeekListener?.call(this);
Repository: wagtail/wagtail
File: client/src/controllers/RevealController.ts (TypeScript)

Code before:
  close() {
    this.closedValue = true;
  }

  /**
   * Open (show) the reveal content.
   */
  open() {
    this.closedValue = false;
  }

  peek() {
    if (this.closedValue) {
      this.peekingValue = true;
      this.open();
    }
  }

  /**
   * Toggle the open/closed state of the controller, accounting for the peeking state (visually open, but not 'fixed' as open).
   * The updated closed value will be stored in local storage if available.
   */
  toggle() {
    const newCLosedValue = !this.closedValue;

    this.stored = this.peekingValue ? false : newCLosedValue;

    if (this.peekingValue) {
      this.peekingValue = false;
      // if peeking and toggle clicked, is already open
      // visually set the icon to the opened variant so it can be closed.
      this.updateToggleIcon(false);
      return;
    }
    this.closedValue = !this.closedValue;
  }

  /**
   * Collects all toggles, those controlled by this controller and any external
   * that have an aria-controls references to any content target elements.
   */
  get toggles() {
    const toggles = this.contentTargets
      .map((content) => content.id)
      .flatMap((id) =>
        Array.from(
          document.querySelectorAll<HTMLButtonElement>(
            `[aria-controls~="${id}"]`,
          ),
        ),
      )

Review comment (suggestion):
Let's streamline this a bit.

```suggestion
    this.stored = this.peekingValue ? false : !this.closedValue;
```

Code after:
        },
        target,
      });
    });
  }

  /**
   * Close (hide) the reveal content.
   */
  close() {
    this.closedValue = true;
  }

  /**
   * Open (show) the reveal content.
   */
  open() {
    this.closedValue = false;
  }

  peek() {
    if (this.closedValue) {
      this.peekingValue = true;
      this.open();
    }
  }

  /**
   * Toggle the open/closed state of the controller, accounting for the peeking state (visually open, but not 'fixed' as open).
   * The updated closed value will be stored in local storage if available.
   */
  toggle() {
    this.stored = this.peekingValue ? false : !this.closedValue;

    if (this.peekingValue) {
      this.peekingValue = false;
      // if peeking and toggle clicked, is already open
      // visually set the icon to the opened variant so it can be closed.
      this.updateToggleIcon(false);
      return;
    }
    this.closedValue = !this.closedValue;
  }

  /**
   * Collects all toggles, those controlled by this controller and any external
   * that have an aria-controls references to any content target elements.
   */
  get toggles() {
    const toggles = this.contentTargets
      .map((content) => content.id)
Repository: wagtail/wagtail
File: client/src/controllers/RevealController.ts (TypeScript)

Code before:
   */
  get stored(): boolean | undefined {
    const key = this.localStorageKey;
    if (!key) return undefined;
    try {
      const value = localStorage.getItem(key);
      if (value === null) return undefined;
      return value === 'closed';
    } catch (error) {
      //  Ignore if localStorage is not available.
    }
    return undefined;
  }

  /**
   * Set the stored state (if closed) of this controller in the local storage.
   * If the store key value is not set, do nothing.
   */
  set stored(isClosed: boolean) {
    const key = this.localStorageKey;
    if (!key) return;
    try {
      if (isClosed) {
        localStorage.setItem(key, 'closed');
      } else {
        localStorage.setItem(key, 'opened');
      }
    } catch (error) {
      // Ignore if localStorage is not available
    }
  }

  disconnect() {
    this.cleanUpPeekListener?.call(this);
  }
}


Review comment (suggestion):
Let's use an enum here and where we use closed, declaring the enum after the imports at the top of the file.

```
enum RevealState {
  OPENED = 'opened',
  CLOSED = 'closed',
}
```

Code after:
    if (!storeValue) return null;
    return ['wagtail', this.identifier, storeValue].join(':');
  }

  /**
   * Get the stored (closed) state of this controller from local storage.
   * If the key is not available, return undefined.
   */
  get stored(): boolean | undefined {
    const key = this.localStorageKey;
    if (!key) return undefined;
    try {
      const value = localStorage.getItem(key);
      if (value === null) return undefined;
      return value === RevealState.CLOSED;
    } catch (error) {
      //  Ignore if localStorage is not available.
    }
    return undefined;
  }

  /**
   * Set the stored state of this controller in the local storage.
   * If the store key value is not set, do nothing.
   */
  set stored(isClosed: boolean) {
    const key = this.localStorageKey;
    if (!key) return;
    try {
      if (isClosed) {
        localStorage.setItem(key, RevealState.CLOSED);
      } else {
        localStorage.setItem(key, RevealState.OPENED);
      }
    } catch (error) {
      // Ignore if localStorage is not available
    }
  }

  disconnect() {
    this.cleanUpPeekListener?.call(this);
  }
}