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

Add function token count comments (rough draft) #14

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sparr
Copy link

@sparr sparr commented May 28, 2023

This needs a lot of work, but the core functionality is working so I'm submitting this for feedback on the implementation so far.

TODO list:

  • replace annotation comment instead of adding another one each time
  • test combined with other shrinko8 functionality, e.g. minify

Stretch goals:

  • additional options
    • don't add annotation comments for nested functions
    • exclude nested functions from parent function counts
    • put annotation comment above existing non-annotation comments

The --annotate option turns this:

-- leading comment
-- comment above func1
function func1(a,b)
 c={a,b,};
 return c[1]
end
-- comment above func2
function func2()
 return true
end
-- trailing comment

into this:

-- leading comment
-- comment above func1
-- T:14 C:47 minC:36
-- minified source:
-- function u(t,r)n={t,r}return n[1]end
function func1(a,b)
 c={a,b,};
 return c[1]
end
-- comment above func2
-- T:5 C:33 minC:27
-- minified source:
-- function n()return true end
function func2()
 return true
end
-- trailing comment

Closes #12

@sparr sparr force-pushed the annotate_function_tokens branch from 117fb00 to 970969f Compare May 28, 2023 14:31
@sparr
Copy link
Author

sparr commented May 28, 2023

Updated top comment with current status. Still very much WIP.

Found a failure mode that I couldn't immediately resolve. This cart:

local x = function() end
function y() x() end

Produces this error:

Traceback (most recent call last):
  File "/home/sparr/src/shrinko8/shrinko8.py", line 216, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/sparr/src/shrinko8/shrinko8.py", line 182, in main
    ok, errors = process_code(ctxt, src, input_count=args.input_count, count=args.count,
  File "/home/sparr/src/shrinko8/pico_process.py", line 169, in process_code
    annotate_code(ctxt, source, root)
  File "/home/sparr/src/shrinko8/pico_annotate.py", line 28, in annotate_code
    apply_node_tree(root, comment_before_function)
  File "/home/sparr/src/shrinko8/pico_annotate.py", line 9, in apply_node_tree
    apply_node_tree(child, func)
  File "/home/sparr/src/shrinko8/pico_annotate.py", line 10, in apply_node_tree
    func(node)
  File "/home/sparr/src/shrinko8/pico_annotate.py", line 22, in comment_before_function
    rename_tokens(ctxt, node, True)
  File "/home/sparr/src/shrinko8/pico_rename.py", line 272, in rename_tokens
    if ident_global in var.scope.used_globals:
AttributeError: 'Scope' object has no attribute 'used_globals'

This is weird because it's happening when pico_annotate asks pico_rename to rename symbols in function y() x() end where it shouldn't be aware of the local x = function() end above, but removing the local definition avoids the error. My initial suspicion is that I've made a mistake in using a single PicoContext for all my operations.

@thisismypassport
Copy link
Owner

This is because pico_rename adds a few attributes (like used_globals) to the scopes it receives and assumes that all scopes then have it. Why do you need to run pico_rename on each function independently? It's unlikely to give sensible results anyway - you should run it on the entire thing.

@sparr
Copy link
Author

sparr commented May 30, 2023

Running it on the whole thing will use two letter identifiers for files with more than 26 globals (or outer scope locals), while doing one function at a time will use single characters almost always.
I'll try to pay more attention to when the scopes get those new attributes and when they are checked for.

@thisismypassport
Copy link
Owner

thisismypassport commented May 30, 2023

It'd probably make sense to move the used_globals/locals definitions to the Scope class itself (its init) instead of monkey patching it - should resolve this exception.

@thisismypassport
Copy link
Owner

I moved used_locals/globals (& now a very-rarely-needed used_members) to be lazy properties on Scope now, FYI.

@sparr
Copy link
Author

sparr commented Jun 23, 2023

Thanks! That resolved the problems I was having.
I think the core functionality here is working now.

The next thing I need to do is get it to recognize its own comments so it will overwrite them and be idempotent instead of adding more new comments each time.

And then there's discussion about how to expose it with new options. I'm thinking of one option that adds the -- T:9 C:86 minC:29 annotation, and another that also adds the minified source comment.

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.

Feature proposal: annotate source with token/character counts for functions
2 participants