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.

Sign up for GitHub

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

Jump to bottom

3287-Redefine isClass in Trait #3288

Merged
merged 1 commit into from Jun 18, 2019
Merged

3287-Redefine isClass in Trait #3288

merged 1 commit into from Jun 18, 2019

Conversation

Alisu
Copy link
Contributor

@Alisu Alisu commented May 2, 2019

Should correct issue #3287

@Alisu Alisu changed the title Redefine isClass in Trait 3287-Redefine isClass in Trait May 2, 2019
MarcusDenker
MarcusDenker previously approved these changes May 4, 2019
View reviewed changes
Copy link
Member

@MarcusDenker MarcusDenker left a comment

Choose a reason for hiding this comment

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

Yes, that looks like a good change. no tests break with this

@dionisiydk
Copy link
Contributor

Looking at senders of #isClass it will affect some existing behaviour.
For example CompiledMethod>>#referencedClasses will not return referenced traits. Maybe it is expected change but some tools could rely on it.

Generally considering that Trait is now subclass of Class I feel it confusing that isClass will return false while "Trait inheritsFrom: Class" will be true

@MarcusDenker MarcusDenker dismissed their stale review May 6, 2019 16:43

see comment denis

@guillep guillep requested review from tesonep and guillep May 7, 2019 12:44
tesonep
tesonep approved these changes May 10, 2019
View reviewed changes
Copy link
Collaborator

@tesonep tesonep left a comment

Choose a reason for hiding this comment

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

I like the change, if there is need for changes they should be added as tests.
The method in CompiledMethod should use #isBehavior if that is required.

@stale
Copy link

stale bot commented May 30, 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.

@stale stale bot added the stale label May 30, 2019
@MarcusDenker MarcusDenker removed the stale label Jun 6, 2019
astares
astares approved these changes Jun 14, 2019
View reviewed changes
Copy link
Member

@astares astares left a comment

Choose a reason for hiding this comment

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

OK

@tesonep tesonep merged commit 407ae1e into pharo-project:Pharo8.0 Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tesonep tesonep tesonep approved these changes

@astares astares astares approved these changes

@MarcusDenker MarcusDenker MarcusDenker left review comments

@guillep guillep Awaiting requested review from guillep

Assignees
No one assigned
Labels
Status: Tests passed please review!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Alisu @dionisiydk @tesonep @MarcusDenker @astares