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

Made some changes #4

Closed
wants to merge 10 commits into from
Closed

Made some changes #4

wants to merge 10 commits into from

Conversation

brunotvs
Copy link

No description provided.

@HiPhish
Copy link
Owner

HiPhish commented Jan 1, 2025

Thanks for contributing, but can you please provide some more commentary as to what you are trying to accomplish? This PR is just a string of changes, and I cannot see what the big picture is without digging into the details. I general it is better to open multiple PRs for multiple topics, then we can discuss them individually and merge them independently of one another.

It will take me a while to go through the commits (feeling quite under the weather), so please allow me some time to come back to this.

@brunotvs
Copy link
Author

brunotvs commented Jan 1, 2025

Sorry for the lack of information. I encountered a small issue when running this plugin: when some tests called the print function, an error was thrown. I discovered that this was happening due to how the JSON output was being generated and read.

When there is a print, Busted outputs it alongside the JSON result. To address this, I modified the results module to read only the last line of the generated files. This change is implemented in the feat: reading the last line output by busted commit. Perhaps this should be tagged as fix.

The fix: using data path to run selected tasks commit simply leverages busted cli and the tree:data().path to make to code a little bit cleaner. Perhaps this should be tagged as feat.

The style: using stylua commit includes many changes, but all I did was set up stylua and run it. No code changes were made, just styling adjustments. It also changes vim.tbl_flatten (deprecated) to vim.iter():flatten():totable()

The other commits are mostly adjustments to align with how I personally work with Lua and, to be honest, should have been tagged as "chore."

@HiPhish
Copy link
Owner

HiPhish commented Jan 2, 2025

Please stop pushing commits, you have gone way off track. The original issue is not fixed by changing which line is parsed; take the following test:

it('Does something', function()
	print('lol')
	assert.are_equal(1, 1)
end)

This will still fail because there is no line break after the lol, the output and the test results will be all on one line. I will probably have to write a custom busted output handler to tackle the issue at its source. I'll first create a test to replicate the original issue, then define an output handler which either writes output and results to separate files or adds a separator before the result.

On top of that you have made a number of unrelated changes to the project, changing code style and tooling. Please do not do that, the code style may not be to your liking, but it is what it is. If you think you have a genuine improvement to the code quality please open a separate PR with just those changes and we can discuss them. In this PR it's just noise that makes it hard to understand what you are trying to accomplish.

@brunotvs brunotvs closed this Jan 2, 2025
@brunotvs
Copy link
Author

brunotvs commented Jan 2, 2025

I'll close this pull request.

When I forked, I saw that the project had no activity for more than 6 months, so I made the changes to suit my needs. Once I got it to a working state I decided to create this pull request to update upstream.

As per the provided example, seems to work here.

@HiPhish
Copy link
Owner

HiPhish commented Jan 2, 2025

If your fork works for you then keep it around until I fix this issue for good. This will take a while if I want to add tests for the bug. Six months of inactivity is not a big deal for Neovim plugins, sometimes a plugin works well enough that there is no need to update it.

As per the provided example, seems to work here.

Yes, I just noticed it works when using Lua as the Lua interpreter. However, when using Neovim as the Lua interpreter (e.g. for testing Neovim plugins) it has the weird quirk where it will not place a terminating newline after the old output. I use busted mostly for testing Neovim plugins, so that's what I noticed first.

@brunotvs
Copy link
Author

brunotvs commented Jan 2, 2025

I'm using this project to handle Lua projects that are not intended to be Neovim plugins, as I believe the plenary alternative was developed for such use cases.

Regarding tooling, I should not have changed how your original project handles formatting. However, I encountered an issue where, when running formatting commands, the results were inconsistent with the existing formatting.

I know this isn't entirely valid as proof, but I've been using this plugin extensively since making these modifications and haven't encountered any errors. I've been testing with Busted and using local-lua-debugger as the DAP. While I should have written tests, I haven't done so because of a tight timeline with another project. This urgency also led me to modify the files directly rather than opening issues and waiting for solutions.

@brunotvs
Copy link
Author

brunotvs commented Jan 2, 2025

Yes, I just noticed it works when using Lua as the Lua interpreter. However, when using Neovim as the Lua interpreter (e.g. for testing Neovim plugins) it has the weird quirk where it will not place a terminating newline after the old output. I use busted mostly for testing Neovim plugins, so that's what I noticed first.

It worked by setting lua = 'nvim -l' in .busted file

@brunotvs brunotvs mentioned this pull request Jan 3, 2025
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