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

Prepend timestamps with namespace to avoid naming conflicts #754

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mclement18
Copy link

Context:

A class contains two state machines that have conflicting state names and they are both using timestamps. At least one is using a namespace.

Actual behavior:

The timestamp of a particular state from the first state machine will be overridden by the transition to the conflicting state of the second state machine.

New behavior:

The timestamps are prepended by the namespace when provided to avoid name collision.

lib/aasm/base.rb Outdated
@@ -263,12 +263,13 @@ def skip_instance_level_validation(event, name, aasm_name, klass)
end
end

def setup_timestamps(aasm_name)
def setup_timestamps(aasm_name, namespace)
Copy link

Choose a reason for hiding this comment

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

Method setup_timestamps has a Cognitive Complexity of 9 (exceeds 5 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Sep 23, 2021

Code Climate has analyzed commit 61fe78e and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@danielricecodes
Copy link

danielricecodes commented Mar 21, 2022

Bump this for sure, although I think it would be better if this were enabled through options.

timestamps: true should leave the existing behavior alone, while timestamps: { prefix: true } would enable the behavior described herein. As an added bonus, timestamps: { prefix: 'foobar' } could allow any prefix to be defined.

Also, It looks like the README needs to be updated also before this could be merged.

And as a small aside to this gem's maintainers: Cognitive Complexity is a rather subjective nag. 5 is far too sensitive, IMHO.

@j-m-harris
Copy link

+1 for this issue, a timestamps prefix would also be nice (but then isn't that what the namespace already is?)

@route
Copy link

route commented Jul 17, 2023

Current behavior is wrong definitely, it's a bug. Why there's no attention to this PR?

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.

None yet

4 participants