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

CHE-656: Refactor getOpenEditors method from Map to List #706

Merged
merged 1 commit into from
Mar 17, 2016
Merged

Conversation

vinokurig
Copy link
Contributor

@vparfonov please review

* @return opened editor or null if it does not exist
*/
@Nullable
EditorPartPresenter getOpenedEditor(String path);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that passing VirtualFile as param to find the concrete editor will be better. Then to find specific editor you can compare virtual files with given one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer using the path. Why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because you can pass as string anything, when you're expecting specific file, then you couldn't pass any rubbish. It's easier to maintain the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the string is easier and understandable. Using the VirtualFile needs to write the special comparator to find. And what shall I do when I need to find an editor by the file path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then use Path.java. We have specialized class to work with paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need Path.java ? Where is it used? Why did you decide to write so heavy class and used it only once ?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for String path parameter

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you decide to write so heavy class and used it only once ?

I use it in my branch everywhere, where it possible. It provides usefull support for paths. With less code writing. IMO.

For example, if you have smth like /path/to/file and you want to get project from this path, you should create bycicles with substrings, but your path may contains leading slash or not, may contains trailing slash or not. With this utility class you can simple do this: String project = Path.valueOf("path/to/file").segment(1); and get need value. Or if you want to get parent of the specified path, you just do this: Path.valueOf("/some/very/huge/path/to/file").parent() and get /some/very/huge/path/to.

Take a look on eclipse sources, how the developsers use it, and you'll understand. It really helpful for maintaining the resources.

Pros from using Path is that there is a basic path validation for input value and you wont be able to pass invalid value as file path

-1 for String path parameter

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.
+1 for String path parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to use String path parameter. There are many methods where getOpenedEditor(String path) used and this path directly received from their parameters.
@vparfonov what do you think?

@vparfonov
Copy link
Contributor

I like idea with Path, looks useful (similar to java.nio.file). But i don't insist.

@vparfonov
Copy link
Contributor

ok

@vinokurig vinokurig force-pushed the CHE-656 branch 2 times, most recently from 5fab6f0 to 987b07b Compare March 16, 2016 20:08
vinokurig pushed a commit that referenced this pull request Mar 17, 2016
CHE-656: Refactor getOpenEditors method from Map to List
@vinokurig vinokurig merged commit fa376f5 into master Mar 17, 2016
@vinokurig vinokurig deleted the CHE-656 branch March 17, 2016 07:01
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.

None yet

5 participants