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

Stop using ...[] pattern (spread list) #696

Closed
provokateurin opened this issue Sep 2, 2023 · 6 comments
Closed

Stop using ...[] pattern (spread list) #696

provokateurin opened this issue Sep 2, 2023 · 6 comments
Assignees
Labels
package: neon_framework refactoring Something that needs to be refactored

Comments

@provokateurin
Copy link
Member

          Please remove the `...[ ]` pattern where possible as it's really inefficient.

I suggest either using a generator function (the sync* yield stuff, which probably isn't fitting) or add a categories variable and conditionally add elements with categories.add()

Originally posted by @Leptopoda in #600 (comment)

@provokateurin provokateurin added the refactoring Something that needs to be refactored label Sep 2, 2023
@provokateurin provokateurin self-assigned this Sep 8, 2023
@provokateurin
Copy link
Member Author

Instead external lists should be used.

@provokateurin provokateurin removed their assignment Sep 8, 2023
@Leptopoda Leptopoda changed the title Stop using ...[] pattern Stop using ...[] pattern (spread list) Sep 18, 2023
This was referenced Sep 18, 2023
@Leptopoda
Copy link
Member

This now only affects neon* packages as all other occurrences have already been tackled over time.
I have a local branch that just needs some testing and minor touchups thus assigning it to me.

@Leptopoda Leptopoda self-assigned this Nov 2, 2023
@Leptopoda Leptopoda added this to the Neon package release milestone Nov 2, 2023
@provokateurin
Copy link
Member Author

We could add a CI script regex that checks for this. Though there are some legitimate uses (but maybe we can avoid all of them?)

@Leptopoda
Copy link
Member

I think we can avoid most of them as you might be able to see in my upcoming PR (I'll make a draft shortly).

I don't think CI is needed. The dashboard app is the first one developed to our current code style and standard and it already doesn't make use of this pattern. And for now you can count on my reviews as I really hate this pattern when missused.

@provokateurin provokateurin modified the milestones: Neon framework package release, Release App 1.0 Jan 16, 2024
@Leptopoda
Copy link
Member

I don't think that this needs to be in the first app release.
It should be in the neon_framework release but the remaining performance implications are minor and it is easier to tackle this issue differently (unified loading indicator and reworking the settings screen would remove most of the remaining cases)

@provokateurin
Copy link
Member Author

This is rarely used anymore and the few remaining places are not performance sensitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: neon_framework refactoring Something that needs to be refactored
Projects
None yet
2 participants