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

Provide error information directly when class definition loading fails #16898

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions src/Monticello/MCPackageLoader.class.st
Original file line number Diff line number Diff line change
Expand Up @@ -146,32 +146,31 @@ MCPackageLoader >> loadClassDefinitions [
When loading a class it is possible that it fails. The reason is that a class can use a trait from this package, but the dependency sorter of Monticello does not sort those traits before the classes using it.
In that case we are implementing a retry mechanism. If error persist, the exception message is logged to the Transcript"

| errorDefinitions |
errorDefinitions := OrderedCollection new.
| errors |
errors := Dictionary new.

"We first load the class definitions and store the potential errors"
[
(additions select: [ :aDefinition | aDefinition isClassDefinition ])
do: [ :aDefinition |
[ aDefinition load ]
on: Error
do: [ errorDefinitions add: aDefinition ] ]
do: [ :error | errors at: aDefinition put: error ] ]
displayingProgress: 'Loading classes...'.

"Until we succed to fix errors, we try to install them"
[
| previousErrors |
previousErrors := errorDefinitions copy.
previousErrors := errors copy.
previousErrors
do: [ :aClassDefinition |
errorDefinitions remove: aClassDefinition.
keysDo: [ :aClassDefinition |
errors removeKey: aClassDefinition.
[ aClassDefinition load ]
on: Error
do: [ :ex | errorDefinitions add: aClassDefinition ] ]
displayingProgress: 'Reloading erroneous definitions...'.
previousErrors size = errorDefinitions size ] whileFalse ] ensure: [ "Finally we warn about the errors we could not fix."
errorDefinitions ifNotEmpty: [
self warnAboutErrors: errorDefinitions ] ]
do: [ :error | errors at: aClassDefinition put: error ] ].
previousErrors size = errors size ] whileFalse ] ensure: [ "Finally we warn about the errors we could not fix."
errors ifNotEmpty: [
self warnAboutErrors: errors ] ]
]

{ #category : 'patch ops' }
Expand Down Expand Up @@ -242,16 +241,19 @@ MCPackageLoader >> warnAboutDependencies [
]

{ #category : 'private' }
MCPackageLoader >> warnAboutErrors: errorDefinitions [
MCPackageLoader >> warnAboutErrors: errorsDict [

self notify: (String streamContents: [ :s |
s
nextPutAll: 'The following definitions had errors while loading. Press Proceed to try to load them again (they may work on a second pass):';
cr.
errorDefinitions do: [ :ea |
errorsDict keysDo: [ :ea |
s
space;
space;
nextPutAll: ea summary;
nextPutAll: ' (';
nextPutAll: ((errorsDict at: ea) description asString);
nextPutAll: ')';
Comment on lines +250 to +257
Copy link
Member

@jecisc jecisc Jul 15, 2024

Choose a reason for hiding this comment

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

Suggested change
errorsDict keysDo: [ :ea |
s
space;
space;
nextPutAll: ea summary;
nextPutAll: ' (';
nextPutAll: ((errorsDict at: ea) description asString);
nextPutAll: ')';
errorsDict keysAndValuesDo: [ :definition :error |
s
space;
space;
nextPutAll: definition summary;
nextPutAll: ' (';
nextPutAll: error description asString;
nextPutAll: ')';

Copy link
Member

Choose a reason for hiding this comment

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

@jecisc may be we should do your changes and integrate this PR

Copy link
Member

Choose a reason for hiding this comment

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

I don't have the rights to push my changes to this PR. I'll just integrate it. My changes were for readability only

cr ] ])
]