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

Add strict option #9

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Add strict option #9

merged 5 commits into from
Oct 10, 2024

Conversation

abelino
Copy link
Collaborator

@abelino abelino commented Oct 6, 2024

Referencing #8

Adds ability to add the strict option locally to a struct's attribute or globally, applying to an entire struct.

struct MQTT.AddDevice.V1

name "add_device"

attribute :name, :string, strict: true
struct MQTT.AddDevice.V1

name "add_device"

strict true

attribute :name,    :string
attribute :address, :string, strict: false

@abelino abelino added the feature label Oct 6, 2024
@abelino abelino requested a review from amclain October 6, 2024 22:10
@abelino abelino self-assigned this Oct 6, 2024
Copy link
Member

@amclain amclain left a comment

Choose a reason for hiding this comment

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

Updates to the readme are missing 😉

@@ -15,7 +15,6 @@ defmodule Speck.MixProject do
deps: deps(),
docs: docs(),
dialyzer: [
ignore_warnings: "dialyzer.ignore.exs",
Copy link
Member

Choose a reason for hiding this comment

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

⛔ (required)

Why was this removed? It's an important project file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The file wasn't removed but renamed to .dialyzer_ignore.exs which is the default name dialyxir looks for in a project. The way some of the source code is written in dialyxir also suggests usage of ignore_warnings is considered legacy. There is a bug in dialyxir right now which flat out ignores the ignore_warnings opt and so even w/ ignore_warnings defined it'll still look for .dialyzer_ignore.exs hence the rename.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pinned the version to 1.4.3 avoiding the issue entirely.

Copy link
Member

@amclain amclain Oct 9, 2024

Choose a reason for hiding this comment

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

✅ Hm, I wonder if .dialyzer_ignore.exs became a thing after I created the design pattern around the other file. Since that's what their docs say to use, that's a good reason for us to switch over to doing it that way. Our project template will also need to be updated.

@abelino abelino force-pushed the add-strict-option branch from 5e9d103 to ba35141 Compare October 8, 2024 17:34
@abelino abelino force-pushed the add-strict-option branch from ba35141 to 956c9f5 Compare October 8, 2024 17:43
@abelino abelino requested a review from amclain October 8, 2024 18:09
@abelino abelino force-pushed the add-strict-option branch from bf1d525 to 487883c Compare October 9, 2024 17:24
@abelino abelino merged commit 33e4bef into main Oct 10, 2024
4 checks passed
@abelino abelino deleted the add-strict-option branch October 10, 2024 18:35
@abelino abelino mentioned this pull request Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants