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 some support for public keyword #886

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Conversation

exaexa
Copy link
Contributor

@exaexa exaexa commented Nov 10, 2024

Currently the public prints out a warning and formats in a pretty bad way (the space after public is removed, essentially merging the identifiers as public apublica).

This is an attempt to make it work, but it does not yet. Opening this for tracking; feel free to point out obvious issues that I missed. :) edited, seems to work now.

@exaexa
Copy link
Contributor Author

exaexa commented Nov 10, 2024

EDIT: boom it seems to work for me now, the spaces are back.

@exaexa exaexa changed the title add some support for public keyword (wip) add some support for public keyword Nov 10, 2024
Copy link
Owner

@domluna domluna left a comment

Choose a reason for hiding this comment

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

LGTM! Can you add tests? Then this looks good to go.

@exaexa
Copy link
Contributor Author

exaexa commented Nov 19, 2024

@domluna Hi! I added a very simple one following the other export test in tests, but is there any good way to add a comprehensive testset? I saw there's whole other packages imported in test/ but these don't seem to contain any public keywords yet. So I'm kinda out of inspiration. :D

test/files/noformat2.jl Outdated Show resolved Hide resolved
@exaexa
Copy link
Contributor Author

exaexa commented Nov 23, 2024

@domluna btw should I rebase this over the main branch? Having this over the other batch of fixes sounds sensible to me but should have asked. :D

@domluna
Copy link
Owner

domluna commented Nov 27, 2024

It should be fine - I'm just going to squash and merge it. Anything else you wanted to add or all good from your end?

@exaexa
Copy link
Contributor Author

exaexa commented Nov 28, 2024

AFAIK all good, the main issue was not to mess up the formatting of public which looks achieved to me. If there are other concerns appearing I'll start a new PR :)

Thanks!

@domluna domluna merged commit 5f83396 into domluna:dl/v2-fixes Nov 28, 2024
domluna pushed a commit that referenced this pull request Dec 1, 2024
* add some support for public keyword

* fix the kset memberships for `public`

* add a simple test
domluna added a commit that referenced this pull request Dec 26, 2024
* fixes kwargs issues and returns being added to do expressions - we're just not doing that completely anymore

* fix more issues

* fix bug related to the whitespace_ops_in_indices option

* revert multiline comments handling for now

* add some support for `public` keyword (#886)

* add some support for public keyword

* fix the kset memberships for `public`

* add a simple test

* run public keyword test on 1.11+

* add always return for do blocks with yas style

---------

Co-authored-by: Mirek Kratochvil <exa.exa@gmail.com>
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