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

fixed identifier assigner added #45

Open
wants to merge 1 commit into
base: release-candidate
Choose a base branch
from

Conversation

matiasgiorda1999
Copy link

changes for add the fixed identifier assigner

@codecov
Copy link

codecov bot commented Dec 27, 2022

Codecov Report

Merging #45 (65f5b0b) into release-candidate (b4cdc28) will not change coverage.
The diff coverage is 100.00%.

@@                 Coverage Diff                 @@
##           release-candidate       #45   +/-   ##
===================================================
  Coverage             100.00%   100.00%           
===================================================
  Files                     35        35           
  Lines                   1376      1395   +19     
===================================================
+ Hits                    1376      1395   +19     
Impacted Files Coverage Δ
...ce/Boardwalk-Tests/IdentifierAssignerTest.class.st 100.00% <100.00%> (ø)
source/Boardwalk/IdentifierAssigner.class.st 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gcotelli
Copy link
Member

@mtabacman I will let you comment on this. I don't get why we need to configure the IdentifierAssigner to use a fixed identifier instead of just settings the fixed id: on the component. But probably it is because I'm missing the usage context of this suggestion.

@jvanecek
Copy link
Member

@mtabacman I will let you comment on this. I don't get why we need to configure the IdentifierAssigner to use a fixed identifier instead of just settings the fixed id: on the component. But probably it is because I'm missing the usage context of this suggestion.

@gcotelli Indeed Willow's ComponentAttributeCommand allows to set the id attribute (use case that applies when you need to know in advance the component's id), but the WebInteractionInterpreter and other objects that make use of the identifier obtains it using the #identifierOn: message, which ends up responded by the identifierAssigner of the view and has no knowledge of the id you set via ComponentAttributeCommand.

I guess we could think of an enhancement of ComponentAttributeCommand to notify the assigner when the first wants define the view's id, but we wanted to keep this object simple enough.

@mtabacman
Copy link
Member

The changes are not compatible with the responsibilities expected of the object.
If we want a fixed identifier, we should create a polymorphic object in whatever project will make use of it and connect it from there.
These changes without an explicit use do not seem useful.

Copy link
Member

@mtabacman mtabacman left a comment

Choose a reason for hiding this comment

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

IF we decide that Boardwalk for some reason needs something polymorphic with an IdentifierAssigner that always assigns the same id, it should be made using a new object with compatible protocol, but not with a new message that changes the IdentifierAssigner to do nothing at all and be filled with configurable nothingness :-)

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.

4 participants