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

Make PsiStatements create a hard copy of the passed code block #113

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

yevhenii-nadtochii
Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii commented Jan 31, 2025

This PR fixes implementation of PsiStatements making it more reliable.

Changes are the following:

  1. The class creates a full-fledged copy of the passed code block to prevent unexpected behavior.
  2. firstChild and lastChild properties now throw with the error message when the PsiStatements is empty.
  3. Added shortcuts to concatenate several PsiStatements.

@yevhenii-nadtochii yevhenii-nadtochii marked this pull request as ready for review January 31, 2025 16:41
@yevhenii-nadtochii
Copy link
Contributor Author

@armiol @alexander-yevsyukov PTAL

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

Please see my questions.


/**
* Returns the first child of this element.
*/
public val firstChild: PsiElement = children.first()
public val firstChild: PsiElement
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's firstStatement to avoid the confusion with the braces being also children?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first and last children may be formatting elements.


/**
* Returns the last child of this element.
*/
public val lastChild: PsiElement = children.last()
public val lastChild: PsiElement
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment to the name and the type of the field. Is it a statement what we're getting here?

public val firstChild: PsiElement = children.first()
public val firstChild: PsiElement
get() {
// Checking for `>2` because we are not interested in surrounding braces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, it is your particular use-cases, in which the access to the braces isn't needed. In other cases such access may be required.

Instead of overriding the firstChild and lastChild altogether, I would rather have another pair of properties which would serve your needs.

Copy link
Contributor Author

@yevhenii-nadtochii yevhenii-nadtochii Jan 31, 2025

Choose a reason for hiding this comment

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

In other cases such access may be required.

Then this type should not be used. Original PsiCodeBlock represents statements with braces.

These methods are not overridden because the class doesn't extend PsiElement. Otherwise, it'd be quite complex.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the suggested property names may need revision. We do return children.first().nextSibling. It is NOT firstChild. It is something else.

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

Let's discuss via video call. I'm not sure about the whole idea about these properties.

public val firstChild: PsiElement = children.first()
public val firstChild: PsiElement
get() {
// Checking for `>2` because we are not interested in surrounding braces.
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the suggested property names may need revision. We do return children.first().nextSibling. It is NOT firstChild. It is something else.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@yevhenii-nadtochii please feel free to re-request the review once the changes discussed vocally are implemented. Thanks!

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.

3 participants