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

4629-ClassDefinitionParser-should-allow-global-message-node-receivers-for-slots #4630

Conversation

@astares
Copy link
Member

astares commented Sep 16, 2019

ClassDefinitionParser should allow global message node receivers for slots

fix #4629

@dionisiydk

This comment has been minimized.

Copy link
Contributor

dionisiydk commented Sep 16, 2019

We need a test

@astares

This comment has been minimized.

Copy link
Member Author

astares commented Sep 16, 2019

As you see from the discussion on the ML this is not yet decided which path Pharo will go with slots in the class definition. First Marcus will work on the slot compositions and then we will see which way we go with the parser.

Currently the CDCParser is unfinished but already integrated. It leaves the context menu broken when a slot object definition is done using a class message - which makes it unusable to work with Calypso.

For now this PR just avoids the debug context menu - so yes it is a workaround for the time being.
A test makes sense after having a full syntax decision.

@dionisiydk

This comment has been minimized.

Copy link
Contributor

dionisiydk commented Sep 17, 2019

Hi @astares .

I disagree with you. Whatever syntax we will choose the class parser should not fail on such expressions.

Currently the only user of class parser is "Calypso commands machinery". But idea is to use it for class creation. CMD+s on class creation tab in the browser will parse definition written by user and it should not fail on incorrect syntax.

So I think we need the test covering slot definition part starting with global name

@astares

This comment has been minimized.

Copy link
Member Author

astares commented Sep 17, 2019

@dionisiydk

As I said: I agree that we need a test - but this PR should go in now as currently the context menu is broken in latest image and Calypso unusable (or even write a sample to provide a test)

Opened a separate issue for the test addition: #4636

…global-message-node-receivers-for-slots
@stale

This comment has been minimized.

Copy link

stale bot commented Oct 7, 2019

It is hard to review old PRs so this PR has been marked as stale since there has been no activity the last 20 days. It will be closed in 10 days if no further activity occurs. A simple comment will reactivate the PR, but please also consider updating the PR to the latest SNAPSHOT build to make it easier for reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.