-
Notifications
You must be signed in to change notification settings - Fork 0
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
Version number comparison & fix chained version renames #134
Version number comparison & fix chained version renames #134
Conversation
d05d272
to
5edc84c
Compare
# TODO: delete | ||
test "chained version" do | ||
assert classname("u-version-5-0-0", {5, 0, 1}) == "u-version-5-0-0" | ||
assert classname("u-version-5-0-0", {5, 0, 0}) == "u-version-5-0-0" | ||
assert classname("u-version-5-0-0", {4, 9, 0}) == "u-version-4" | ||
assert classname("u-version-5-0-0", {4, 4, 0}) == "u-version-4" | ||
assert classname("u-version-5-0-0", {4, 3, 0}) == "u-version-4" | ||
assert classname("u-version-5-0-0", {4, 2, 0}) == "u-version-4" | ||
assert classname("u-version-5-0-0", {4, 1, 0}) == "u-version-4" | ||
assert classname("u-version-5-0-0", {4, 0, 0}) == "u-version-4" | ||
assert classname("u-version-5-0-0", {3, 9, 9}) == "u-version-2" | ||
assert classname("u-version-5-0-0", {3, 0, 0}) == "u-version-2" | ||
assert classname("u-version-5-0-0", {2, 0, 0}) == "u-version-2" | ||
assert classname("u-version-5-0-0", {1, 6, 0}) == "u-version-1-5" | ||
assert classname("u-version-5-0-0", {1, 5, 0}) == "u-version-1-5" | ||
assert classname("u-version-5-0-0", {1, 4, 0}) == "u-version-1-3" | ||
assert classname("u-version-5-0-0", {1, 3, 0}) == "u-version-1-3" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be deleted after the code review, together with the corresponding classes in bitstyles.ex
. I added this test to change a check that the new chaining behavior works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a great idea to keep it. You could add the class mapping in as a compile time switch config
(add_version_test_classes
), so they do not end up in the compiled versions for prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. let's say I do something like
# config.exs
if config_env() in [:test, :e2e] do
config :bitstyles_phoenix, add_version_test_classes: true
end
I'm not even sure how to use it as a compile-time check in something like this:
defp downgrade_classname(class, target_version, current_version)
when should_downgrade_from({4, 0, 0}, target_version, current_version) do
mapping =
case class do
"u-overflow-y-auto" -> "u-overflow--y"
# I can't sqeezee an if expression here
"u-version-4" -> "u-version-2"
_ -> class
end
end
But I'm also thinking: if somebody adds bitstyles_phoenix to their project and compiles their project in test env (to run tests), then bitstyles_phoenix will include those 'test' classes in its source. Which means, if the user of the library is relying on class substitution in their app, and for some weird reason uses a class like u-version-4
, then that class will get substituted in their app 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependencies are always compiled in prod
ENV except when you state it on the dependency directly.
https://hexdocs.pm/mix/Mix.Tasks.Deps.html#module-dependency-definition-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For compile time switches you can even have a if block around the whole function definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 7a0fdc5
(#134)
7e6333a
to
b3ce605
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm 💚🚀 Nice work
Co-authored-by: Andreas Knöpfle <[email protected]>
Resolves #123
TODO: