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

[INTERNAL][E] Introduce reference point based resource implementation #1023

Merged
merged 2 commits into from
Aug 4, 2020

Conversation

tobous
Copy link
Member

@tobous tobous commented Jun 3, 2020

This is my current draft for the new Eclipse resource implementation. The details of the implementation might still change a bit if I encounter any trouble while migrating Saros/E to the new resource implementation but should be stable in general.

A full review of the class is not sensible quite yet (as it still might change), but I would appreciate some feedback on the general design of the implementation.

Commits

[INTERNAL][E] Introduce reference point based resource implementation

Adds new resource implementation based on the reference point model.

The new resource implementation somewhat resembles the new IntelliJ
resource implementation. This is due to both implementations now
operating on the same resource model and being bound by the same
restrictions (i.e. the Eclipse implementation now also having to deal
with relative resources instead of solely relying on the Eclipse
delegates).

Adds ResourceConverter as a replacement for ResourceAdapterFactory.

Marks the old resource implementations as deprecated.

Marks ResourceAdapterFactory as deprecated and removes an unused method.

@tobous tobous added the Area: Eclipse Issue affecting Saros for Eclipse (Saros/E) label Jun 3, 2020
@tobous tobous self-assigned this Jun 3, 2020
@tobous tobous force-pushed the pr/core/clean-up-for-ref-point branch from 870e843 to 571ef80 Compare June 9, 2020 13:15
Base automatically changed from pr/core/clean-up-for-ref-point to master June 9, 2020 13:21
@tobous tobous force-pushed the pr/eclipse/introduce-new-ref-point-resource-impl branch from 039a1dd to f90b06f Compare June 9, 2020 13:22
@tobous
Copy link
Member Author

tobous commented Jun 9, 2020

Rebased onto current master without any interaction.

@tobous tobous force-pushed the pr/eclipse/introduce-new-ref-point-resource-impl branch from f90b06f to a4104ae Compare June 19, 2020 02:06
@tobous
Copy link
Member Author

tobous commented Jun 19, 2020

Addressed review issues. Rebased onto current master. Fixed minor issues with implementation found during migration.

@tobous
Copy link
Member Author

tobous commented Jun 22, 2020

This PR should be ready to review now.

stefaus
stefaus previously approved these changes Jun 23, 2020
Copy link
Contributor

@stefaus stefaus left a comment

Choose a reason for hiding this comment

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

👍 lgtm

@tobous tobous force-pushed the pr/eclipse/introduce-new-ref-point-resource-impl branch from a4104ae to 2f83cce Compare June 24, 2020 21:05
@tobous
Copy link
Member Author

tobous commented Jun 24, 2020

Rebased onto current master without any interaction.

srossbach
srossbach previously approved these changes Jun 25, 2020
Copy link
Contributor

@srossbach srossbach left a comment

Choose a reason for hiding this comment

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

You are still missing the setCharset method for folders. I know it is an Eclipse thing but it should not hurt as it can be ignored by IDE that do not support such things.

@tobous
Copy link
Member Author

tobous commented Jul 1, 2020

You are still missing the setCharset method for folders. I know it is an Eclipse thing but it should not hurt as it can be ignored by IDE that do not support such things.

The encoding handling in general needs improvement. See #912 and #940. As the logic to set the folder charset is currently never used, I would like to keep it the way it is for now. But extending the IFolder interface should not be an issue if it is ever needed/used.

@tobous
Copy link
Member Author

tobous commented Aug 4, 2020

As this has been approved for a while now and does not actually change any of the logic of Saros/E but rather just adds dead code (for now), I decided to merge this now even though the rest of the PR stack has not been fully reviewed.

Adds new resource implementation based on the reference point model.

The new resource implementation somewhat resembles the new IntelliJ
resource implementation. This is due to both implementations now
operating on the same resource model and being bound by the same
restrictions (i.e. the Eclipse implementation now also having to deal
with relative resources instead of solely relying on the Eclipse
delegates).

Adds ResourceConverter as a replacement for ResourceAdapterFactory.

Marks the old resource implementations as deprecated.

Marks ResourceAdapterFactory as deprecated and removes an unused method.
Adjusts the logic to set the encoding for a file to check the direct
parent when trying to inherit the encoding instead of the project. This
was done to match the underlying Eclipse logic which always looks at the
direct parent container instead of the project default.
@tobous tobous force-pushed the pr/eclipse/introduce-new-ref-point-resource-impl branch from 2f83cce to 5172f51 Compare August 4, 2020 03:07
@tobous
Copy link
Member Author

tobous commented Aug 4, 2020

Rebased onto current master without any interaction.

@saros-infrastructure
Copy link

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- eclipse/src/saros/filesystem/EclipseReferencePointImpl.java  7
- eclipse/src/saros/filesystem/EclipseFolderImplV2.java  7
- eclipse/src/saros/filesystem/ResourceConverter.java  5
- eclipse/src/saros/filesystem/EclipseFileImplV2.java  4
- eclipse/src/saros/filesystem/EclipseResourceImplV2.java  5
         

Clones added
============
- eclipse/src/saros/filesystem/EclipseFileImplV2.java  2
- eclipse/src/saros/filesystem/EclipseFileImpl.java  2
         

See the complete overview on Codacy

@tobous tobous merged commit ee6e864 into master Aug 4, 2020
@tobous tobous deleted the pr/eclipse/introduce-new-ref-point-resource-impl branch August 4, 2020 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Eclipse Issue affecting Saros for Eclipse (Saros/E)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants