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

📎 Dynamic code hints: warn when creating a multiple definition #1859

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

Conversation

fonsp
Copy link
Owner

@fonsp fonsp commented Jan 27, 2022

Schermopname.2022-01-27.om.00.59.24.mov

The warning system is more general, and can be used for other syntax-matching hints.

TODOs:

  • Methods warns (canonicalize on the frontend ?)
  • Merge lint gutter and line number

@github-actions
Copy link
Contributor

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/fonsp/Pluto.jl", rev="warn-double-definitions")
julia> using Pluto

@Pangoraw
Copy link
Collaborator

Pangoraw commented Jan 28, 2022

Very cool! 🔥
may be a good use case for @codemirror/lint. what do you think ?
linter

warn-double-definitions...warn-double-definitions-codemirror-lint

@icweaver
Copy link
Contributor

icweaver commented Jan 28, 2022

ayyy, it even knows about DataFrames! (just function calls in general?)

using DataFrames
hello = 123
DataFrame(hello = rand(10))  # All good in the neighborhood

although it will grumble while in the process of making a named tuple, but maybe that should be preferred?

hmm-2022-01-28_16.41.29.mp4

@fonsp
Copy link
Owner Author

fonsp commented Feb 10, 2022

This is now merged! #1859 (comment)

@Pangoraw
Copy link
Collaborator

Pangoraw commented Feb 15, 2022

disabling

i am looking into overlaying the warning gutter with line numbers. (disabling 6892d7a in multiple definitions can be moved to another pr)

@fonsp
Copy link
Owner Author

fonsp commented Feb 15, 2022

frontend tests will give ✅ again if you update from main!

@fonsp
Copy link
Owner Author

fonsp commented Feb 15, 2022

Awesome work! This is great for people on a touch screen (can't hover)

@fonsp
Copy link
Owner Author

fonsp commented Feb 15, 2022

Wait...... did you just secretly fix #1386............ WOW

Can you put that on main? Amazing!!

@Pangoraw
Copy link
Collaborator

Wait...... did you just secretly fix #1386............ WOW

Can you put that on main? Amazing!!

there is still a sync bug (when you re-enable a cell), i'll update when #1895 is merged

@fonsp
Copy link
Owner Author

fonsp commented Feb 15, 2022

Cool! What do you need from that PR? Or just avoiding merge conflicts

@fonsp
Copy link
Owner Author

fonsp commented Mar 29, 2022

@Pangoraw can you move the backend changes to a new PR? Did you say that it has performance issues, or was that about something else?

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.

3 participants