Skip to content
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

🏹 Upgrade Elixir version #1024

Merged
merged 17 commits into from
Oct 29, 2024
Merged

🏹 Upgrade Elixir version #1024

merged 17 commits into from
Oct 29, 2024

Conversation

Whoops
Copy link
Collaborator

@Whoops Whoops commented Oct 24, 2024

Summary of changes

Asana Ticket: 🏹 Upgrade Elixir version

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

@@ -56,8 +56,7 @@ if config_env() == :prod do
root_source_code_path: File.cwd!(),
tags: %{
env: sentry_env
},
included_environments: [sentry_env]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -285,4 +285,12 @@ defmodule Arrow.DisruptionRevision do
changeset
end
end

defp date_range(start_date, end_date) do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Address new deprecation warnings where it's required to provide a step of -1 if end_date > start_date

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the code below is correct but what you meant in this comment was start_date > end_date for step of -1 (so it enumerates backwards in time from start_date to end_date)

mix.exs Outdated
{:httpoison, "~> 1.6"},
{:ja_serializer, github: "mbta/ja_serializer", branch: "master"},
{:httpoison, "~> 2.2"},
{:ja_serializer, "~> 0.18.0"},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our version of ja_serializer has diverged from upstream, but also not been touched in 3 years. Best I could tell Arrow does not appear to rely on any MBTA specific behaviors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in support of this change since our version hasn't been maintained.

Was this upgrade of ja_serializer required as part of upgrading the elixir version though?

Can we split this change into it's own separate PR? If we notice any unexpected changes (like with the gtfs_creator build when parsing API results, performance issues with endpoints, whatever), it'd be easier to isolate that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like it can be. Some of these depredations were forced since we compile with --warnings-as-errors in the test phase, but while keeping ja_serializer generates warnings during tests, it seems to compile happily. I've split it into #1026

@@ -141,18 +134,6 @@ defmodule ArrowWeb.ShapeControllerTest do
assert html_response(conn, 200) =~ "Components.ShapeViewMap"
end

@tag :authenticated_admin
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was fixed in this pr, that was released in 1.3.0, so these now parse correctly.

@@ -17,7 +17,7 @@ defmodule ArrowWeb.CoreComponents do
use Phoenix.Component

alias Phoenix.LiveView.JS
import ArrowWeb.Gettext
use Gettext, backend: ArrowWeb.Gettext
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -51,11 +51,6 @@ defmodule ArrowWeb.ShapeController do
conn
|> put_flash(:errors, reason)
|> render(:new_bulk, errors: reason, shapes_upload: reset_upload)

error ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dialyzer flags this clause as unreachable, and I agree. parse_kml_from_file and shapes_from_kml will always return {:ok, _} or {:error, _}, and changeset will always return a changeset, so will always match.

@@ -14,7 +14,7 @@ defmodule Arrow.MixProject do
releases: releases(),
dialyzer: [
plt_add_apps: [:mix],
plt_add_deps: :transitive,
plt_add_deps: :app_tree,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

flags: [
:unmatched_returns
],
ignore_warnings: ".dialyzer.ignore-warnings"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dialyzer complained about could not read file stats ".dialyzer_ignore.exs": no such file or directory as long as this was present. I didn't figure out why, but we also aren't ignoring warnings (.dialyzer.ignore-warnings is blank) so I removed it.

@Whoops Whoops requested review from a team and meagharty and removed request for a team October 24, 2024 21:17
@@ -39,6 +39,7 @@ COPY config/config.exs config/
COPY config/prod.exs config/

RUN mix deps.compile
RUN mix sentry.package_source_code
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -40,7 +40,7 @@ defmodule ArrowWeb.StopControllerTest do
conn = post(conn, ~p"/stops", stop: invalid_attrs)
assert redirected_to(conn) == ~p"/stops/new"

assert get_flash(conn, :errors) ==
assert conn.assigns.flash["errors"] ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert conn.assigns.flash["errors"] ==
assert Phoenix.Flash.get(conn.assigns.flash, :errors) ==

mix.exs Outdated
{:httpoison, "~> 1.6"},
{:ja_serializer, github: "mbta/ja_serializer", branch: "master"},
{:httpoison, "~> 2.2"},
{:ja_serializer, "~> 0.18.0"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in support of this change since our version hasn't been maintained.

Was this upgrade of ja_serializer required as part of upgrading the elixir version though?

Can we split this change into it's own separate PR? If we notice any unexpected changes (like with the gtfs_creator build when parsing API results, performance issues with endpoints, whatever), it'd be easier to isolate that way.

Copy link
Contributor

@meagharty meagharty left a comment

Choose a reason for hiding this comment

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

🎉

@@ -285,4 +285,12 @@ defmodule Arrow.DisruptionRevision do
changeset
end
end

defp date_range(start_date, end_date) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the code below is correct but what you meant in this comment was start_date > end_date for step of -1 (so it enumerates backwards in time from start_date to end_date)

@Whoops Whoops merged commit 8c2acde into master Oct 29, 2024
28 checks passed
@Whoops Whoops deleted the wh-update-elixir branch October 29, 2024 17:45
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.

2 participants