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

feat(standard): one import for most of the standard lib #4504

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

Rathoz
Copy link
Collaborator

@Rathoz Rathoz commented Aug 9, 2024

Summary

What's everyone's opinion on this?

How did you test this change?

@mbergen
Copy link
Collaborator

mbergen commented Aug 9, 2024

Not familiar enough with the system, does this have any sizable impact on performance over importing just the ones needed?

Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

not opposed
means lots of cleanup though

@Rathoz
Copy link
Collaborator Author

Rathoz commented Aug 9, 2024

Not familiar enough with the system, does this have any sizable impact on performance over importing just the ones needed?

Negligible, Vogan did some tests a long time ago on it.

@LuckeLucky
Copy link
Collaborator

LuckeLucky commented Aug 10, 2024

Maybe something like this https://liquipedia.net/commons/Module:Sandbox/LuckeLucky/test2

Standart

image

Usage

require('Module:Sandbox/LuckeLucky/standart')

local p = {}

function p.test()
	return mw.logObject(DateExt)
end

return p

Result

image

@hjpalpha
Copy link
Collaborator

Maybe something like this https://liquipedia.net/commons/Module:Sandbox/LuckeLucky/test2

Standart

image

Usage

require('Module:Sandbox/LuckeLucky/standart')

local p = {}

function p.test()
	return mw.logObject(DateExt)
end

return p

Result

image

  1. opp lib def should be local no need to access it itself
  2. the linter etc would go crazy because of all the globals so would have to disable the non globals check on that module

Copy link
Collaborator

@hjpalpha hjpalpha left a comment

Choose a reason for hiding this comment

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

from modules sync:

  • generally a good idea
  • we think making it globals might be nice
  • some of the included modules are not used that super often, so we think not including them here is better for now (if we see them being used more often we always can add them then)

standard/standard.lua Outdated Show resolved Hide resolved
standard/standard.lua Outdated Show resolved Hide resolved
standard/standard.lua Outdated Show resolved Hide resolved
standard/standard.lua Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants