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

🔧 Remove attributed type in favor of attributes array on all types #1182

Merged
merged 3 commits into from
May 23, 2024

Conversation

NeunEinser
Copy link
Contributor

Motivation

The attributed type contained a single attribute with another child, which itself could be an attributed type for multiple attributes on a single type.

This can be quite annoying to deal with, especially when different processes still need to be able to access attributes, but also you want to check whether a type node is of a certain type.

With the old concept, you'd need to essentially recurse into the type tree for as long as the current node is still an attributed type.

With this, all types can simply have attributes and they can be dealt with when appropriate.

This is also more consistent with StructTypeField and EnumTypeField.

@SPGoding
Copy link
Member

Side note: would it make sense to do the same for IndexedType as well or are you fine with that

@NeunEinser
Copy link
Contributor Author

Side note: would it make sense to do the same for IndexedType as well or are you fine with that

I think that's included, no?

export interface IndexedType extends McdocBaseType {
	kind: 'indexed'
	parallelIndices: Index[]
	child: McdocType
}

export interface McdocBaseType {
	attributes?: Attribute[]
}

Or did I miss a binder?

@SPGoding
Copy link
Member

I meant if you'd want to remove IndexedType and add the indices to the type base instead, since IndexedType is also recursive. Not sure if that'd make anything easier/harder for you cuz I haven't checked how you implemented things

@NeunEinser
Copy link
Contributor Author

Hmm, maybe I misunderstood indexed?

I understood it as "here is a child type, if this child resolves to a struct, use the index I give to you to index into it and return that sub type instead"

parallelIndices is a list already though, and so is each entry of parallelIndices.

So I don't really know where the recursive definition is , but maybe I am doing something wrong.

@TheAfroOfDoom TheAfroOfDoom self-requested a review May 21, 2024 03:02
@SPGoding
Copy link
Member

I understood it as "here is a child type, if this child resolves to a struct, use the index I give to you to index into it and return that sub type instead"

That's correct.

So I don't really know where the recursive definition is

It's only recursive if there are multiple parallel indices (i.e. brackets) in a row. e.g. Foo[Bar,Baz][Qux][[id]] will be a few nested IndexedTypes.

@NeunEinser
Copy link
Contributor Author

Yeah, I think that should be fine, as my simplify function resolves indexed types (and it should be able to handle them recursively).

The issue with attributes was more that we will need to carry around attributes for later logic, so I can't simplify them away. For indexed types, I can just work with the resolved struct. I guess maybe I still need copy attributes to some resolved types.

Copy link
Member

@misode misode left a comment

Choose a reason for hiding this comment

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

I think it makes sense to do this for attributes and not for indexed types

@misode misode merged commit 6439d5f into SpyglassMC:main May 23, 2024
3 checks passed
TheAfroOfDoom added a commit to TheAfroOfDoom/Spyglass that referenced this pull request May 24, 2024
commit a8431ea
Author: SPGoding <i@spgoding.com>
Date:   Thu May 23 16:54:49 2024 -0500

    🐛 Fix computing relative URIs (SpyglassMC#1177)

commit 0c5e505
Author: Afro <tehafroofdoom@gmail.com>
Date:   Thu May 23 17:53:36 2024 -0400

    🔧 Add `import/no-duplicates` lint rule (SpyglassMC#1181)

commit 50c2185
Author: SPGoding <i@spgoding.com>
Date:   Wed May 22 21:26:34 2024 -0500

    🔥 Remove CommandTreeRegistry (SpyglassMC#1190)

commit 6439d5f
Author: NeunEinser <daniel30797@gmail.com>
Date:   Thu May 23 04:24:54 2024 +0200

    🚧 Remove attributed type in favor of attributes array on all types (SpyglassMC#1182)

commit d99eeb6
Author: SPGoding <i@spgoding.com>
Date:   Wed May 22 21:22:37 2024 -0500

    ⬆️ Upgrade dprint (SpyglassMC#1191)

commit 9a303cc
Author: Afro <tehafroofdoom@gmail.com>
Date:   Wed May 22 22:21:45 2024 -0400

    🔧 Add long timeout arg to unit test launch configuration (SpyglassMC#1189)

commit 2b74e1f
Author: Nico314159 <nicolino.will@gmail.com>
Date:   Wed May 22 13:04:25 2024 -0700

    Implement 1.20.5+ item slots (SpyglassMC#1179)

commit d6c395f
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue May 21 02:58:56 2024 -0500

    ⬆️ Bump rexml from 3.2.5 to 3.2.8 in /docs (SpyglassMC#1153)

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit a1f81c3
Author: NeunEinser <daniel30797@gmail.com>
Date:   Mon May 20 22:15:26 2024 +0200

    🐛 Fix message reporting breaking on empty string (SpyglassMC#1155)
@NeunEinser NeunEinser deleted the mcdoc-better-attributes branch May 25, 2024 06:17
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