Skip to content

fix(ast): preserve emptyline before comment#3

Merged
hleb-rubanau merged 5 commits into
compy-toys:devfrom
hleb-rubanau:dev
May 27, 2026
Merged

fix(ast): preserve emptyline before comment#3
hleb-rubanau merged 5 commits into
compy-toys:devfrom
hleb-rubanau:dev

Conversation

@hleb-rubanau

@hleb-rubanau hleb-rubanau commented May 25, 2026

Copy link
Copy Markdown

Now AST converter tracks emptyline sequences in the source,
and emits a single emptyline into result
whenever such a sequence precedes the comment preceding the node

(NOTE: initial version of this PR was ditched and it was redone from scratch)

@hleb-rubanau hleb-rubanau force-pushed the dev branch 2 times, most recently from e75bd0a to 74341aa Compare May 25, 2026 17:56
@hleb-rubanau hleb-rubanau marked this pull request as draft May 25, 2026 17:57
@hleb-rubanau hleb-rubanau marked this pull request as ready for review May 25, 2026 19:07
@hleb-rubanau hleb-rubanau requested a review from aldum May 25, 2026 19:09
@hleb-rubanau

Copy link
Copy Markdown
Author

@aldum I've completely changed the PR logic to the cleaner and more predictable approach.
Diff is fully replaced, so re-review is needed.

Apologies for the inconvenience!

@aldum aldum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel no confidence that I can game all of this out in my head, please add unit tests that demonstrate this code working as intended.

Comment thread metalua/compiler/ast_to_src.lua Outdated

----------------------------------------------------------------
--- Reset last non-empty source line to the current position
--- @param pos table?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we be more specific about this type? If it's not some existing class, at least a structural type alias would be useful.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would ask for your help/recommendation here then.

In practice it's ad-hoc polymorphic, accepting a union of two types.

Annotation problem is that one of them is not defined explicitl, and for another one I just cannot find type definition within sane time:

  1. clast table as built on line 402 simply does not have type definition or alias
  2. node.lineinfo.first or node.lineinfo.last could be passed -- but I am not sure where the type is defined

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lineinfo is defined here in turn referencing position here
clast is a value, it's type is basically Cursor, but that's in compy, so metalua strictly speaking doesn't know about it.
An ad-hoc union works here, as in

---- @param pos position|{l : integer}

Long term, probably this should be refactored to use a position type, or a new PositionBase (equivalent to Cursor). There's some impedance mismatch because metalua originally had nothing for types (this language server we use might have not even existed yet).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, please check

@hleb-rubanau

Copy link
Copy Markdown
Author

I feel no confidence that I can game all of this out in my head, please add unit tests that demonstrate this code working as intended.

Just added and pushed (see last commit), would love to know your thoughts

Comment thread spec/ast_to_src_spec.lua Outdated
@@ -1,3 +1,5 @@
local fmt=string.format

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one more nit: autoformat

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, for this file only (several other changed, such as ast_to_src itself and spec examples, I did not risk lintering, as they seem to purposefully ignore double-quoting standard for the sake of clarity, and maybe few other conventions too).

For convenience, I've added also added .editorconfig and justfile lint directive (using stylua just because why not). I do not insist them to be a freezed canon, rather some reasonable baseline to start from.

@hleb-rubanau hleb-rubanau requested a review from aldum May 26, 2026 14:43
Now AST converter tracks emptyline sequences in the source,
and emits a single emptyline into result
whenever such a sequence precedes the comment preceding the node
* new test suite added for emptylines manipulation
* spec script adjusted to inject tags at suite and case levels
* justfile enhanced to allow per-tag testing without nodemon
For now, stylua is used as ad-hoc default,
it can be changed later
@hleb-rubanau hleb-rubanau merged commit a42b391 into compy-toys:dev May 27, 2026
1 check passed
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.

2 participants