Skip to content

Rename event to assertion throughout the code (including plural f…#19

Merged
mneumann merged 1 commit intosavi-lang:mainfrom
mneumann:rename-events-to-assertions
Sep 22, 2022
Merged

Rename event to assertion throughout the code (including plural f…#19
mneumann merged 1 commit intosavi-lang:mainfrom
mneumann:rename-events-to-assertions

Conversation

@mneumann
Copy link
Copy Markdown
Contributor

…orms)

Reflecting the change introduced in PR #10 which replaced Spec.Event by Spec.Assert.

We should actually also rename Spec.Assert to Spec.Assertion or is Assert a noun?

…orms)

Reflecting the change introduced in [PR #10][pr-10] which replaced
`Spec.Event` by `Spec.Assert`.

We should actually also rename `Spec.Assert` to `Spec.Assertion` or
is `Assert` a noun?

[pr-10]: #10
@mneumann mneumann requested a review from jemc September 20, 2022 10:11
Copy link
Copy Markdown
Contributor

@jemc jemc left a comment

Choose a reason for hiding this comment

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

Looks good.

Regarding the question of Spec.Assert vs Spec.Assertion I have no strong opinion on the matter, but given that this PR uses assertion everywhere, then yes we should probably use Spec.Assertion for the class name.

@mneumann mneumann merged commit 4e3b4a1 into savi-lang:main Sep 22, 2022
@mneumann mneumann deleted the rename-events-to-assertions branch September 22, 2022 12:00
@mneumann
Copy link
Copy Markdown
Contributor Author

Looks good.

Regarding the question of Spec.Assert vs Spec.Assertion I have no strong opinion on the matter, but given that this PR uses assertion everywhere, then yes we should probably use Spec.Assertion for the class name.

The savi compiler generates instances of Spec.Assert for assert: macros, so we would first have to change this. I am leaving this as is for now.

@jemc
Copy link
Copy Markdown
Contributor

jemc commented Sep 22, 2022

The savi compiler generates instances of Spec.Assert for assert: macros, so we would first have to change this. I am leaving this as is for now.

Yeah, we need to figure out how to clean that up properly.

I don't have any ideas that I'm really sure about, but my best idea at the moment is:

  • Finish getting the manifest-handling code working for allowing a package to export more than one top-level type name (but still encourage most packages to use just one).
  • Add an Assert type to the Spec package (with no Spec. prefix), which creates a Spec.Assertion when called.
  • Make the assert macro always use the Assert type instead of Spec.Assert, allowing potentially any library to export a type named Assert that gets used for assertions within the packages that allow it to be the exporter of that name (remembering that in the design of Savi's namespacing each manifest is meant to govern what names go to which providing packages within the source files of a package defined by that manifest - so it is possible to have Assert be provided by the Spec package in some package source files, while having it be provided by some other package (for example, one providing runtime assertions of some kind) in other source files).

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