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

Exclude components with a "PRIVATE" tag from the component browser #4085

Merged

Conversation

galin-enso
Copy link
Contributor

@galin-enso galin-enso commented Jan 26, 2023

First part of #4962

Pull Request Description

This PR removes components from the component browser if they contain a "PRIVATE" tag. Private components from the current module or project are not hidden.

image

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@galin-enso galin-enso marked this pull request as ready for review January 26, 2023 13:51
app/gui/src/controller/searcher/component/builder.rs Outdated Show resolved Hide resolved
match &component.data {
component::Data::FromDatabase { entry, .. } =>
entry.documentation.iter().any(|doc| match doc {
DocSection::Tag { name, .. } => name == "PRIVATE",
Copy link
Member

Choose a reason for hiding this comment

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

extrct that as const. We are extracting all literals as consts. Plus, check if itdoesnt exists anywhere in the app. Put it in a module with language construct definitions pls.

Copy link
Contributor

Choose a reason for hiding this comment

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

extrct that as const. We are extracting all literals as consts. Plus, check if itdoesnt exists anywhere in the app. Put it in a module with language construct definitions pls.

@wdanilo Are you aware that this is not a language construct, but the documentation tag? I'd put it in the documentation-related crate instead. Or just here.

Copy link
Member

Choose a reason for hiding this comment

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

Documentation is IMO part of language specification. It affects how these things are also accessible from code. IMO this is language-binding thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation is IMO part of language specification. It affects how these things are also accessible from code. > IMO this is language-binding thing.

This is not true AFAIK. You can happily access those methods from the code. The tag just hides them in the Component Browser.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can for now. But we want to introduce a mode where you could not from external modules if you are not enabling "allow unsafe access" or something like that in the language. It is not supported in Engine yet, but it is part of the language.

app/gui/src/controller/searcher/component/builder.rs Outdated Show resolved Hide resolved
app/gui/src/controller/searcher/component/builder.rs Outdated Show resolved Hide resolved
app/gui/src/controller/searcher/component/builder.rs Outdated Show resolved Hide resolved
app/gui/src/controller/searcher/component/builder.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -185,6 +186,9 @@ impl List {
for (id, entry) in ids_and_entries {
self.allowed_favorites.insert(id);
let component = Component::new_from_database_entry(id, entry.clone_ref());
if component_is_private(&component) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this was not part of this task, but we should display private components after key-stroke. Can we add ctrl + option + p here? Just like ctrl + option + <number> changes debug view.

Copy link
Contributor

Choose a reason for hiding this comment

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

The flag readable by controllers could be stored in the IDE controller, because it's accessible from the searcher controller.

Anyway, I would prefer to do it in a separate PR (not necessarily separate task: tasks can be done in more than one PR.

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'll open a new PR for this addition.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Nothing more than @wdanilo comments, see also discussions.

match &component.data {
component::Data::FromDatabase { entry, .. } =>
entry.documentation.iter().any(|doc| match doc {
DocSection::Tag { name, .. } => name == "PRIVATE",
Copy link
Contributor

Choose a reason for hiding this comment

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

extrct that as const. We are extracting all literals as consts. Plus, check if itdoesnt exists anywhere in the app. Put it in a module with language construct definitions pls.

@wdanilo Are you aware that this is not a language construct, but the documentation tag? I'd put it in the documentation-related crate instead. Or just here.

app/gui/src/controller/searcher/component/builder.rs Outdated Show resolved Hide resolved
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

This PR contains binary files. Please be sure to check what files are committed before making PR requests :)

@galin-enso
Copy link
Contributor Author

This PR contains binary files. Please be sure to check what files are committed before making PR requests :)

Oops, that was definitely not my intention. Fixed.

@farmaazon
Copy link
Contributor

QA: No issues spotted. However, I started to wonder if PRIVATE methods from the current module/project should be actually hidden? @wdanilo the question to you: I guess we should display PRIVATE method from the entire currently opened project.

@wdanilo
Copy link
Member

wdanilo commented Feb 8, 2023

@farmaazon thanks for spotting it. You are right, they should not.

@farmaazon
Copy link
Contributor

QA green.

Although, I found one more issue: we still display private methods in the method list inside documentation. But let's not do it as a part of this task, please make another issue for it instead.

@galin-enso
Copy link
Contributor Author

Although, I found one more issue: we still display private methods in the method list inside documentation. But let's not do it as a part of this task, please make another issue for it instead.

Added separate issue: #5635

@galin-enso galin-enso added the CI: Ready to merge This PR is eligible for automatic merge label Feb 13, 2023
@mergify mergify bot merged commit 6967fb7 into develop Feb 13, 2023
@mergify mergify bot deleted the wip/galin-enso/exclude-private-methods-from-cb-184215360 branch February 13, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants