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

source format removes all comments #15

Open
Andrei-Pozolotin opened this issue Nov 9, 2014 · 20 comments
Open

source format removes all comments #15

Andrei-Pozolotin opened this issue Nov 9, 2014 · 20 comments

Comments

@Andrei-Pozolotin
Copy link

problem: Shift+Ctrl+F removes all comments

origin:
http://dadacoalition.org/yedit/

system:

Eclipse Java EE IDE for Web Developers.
Version: Luna Service Release 1 (4.4.1)
Build id: 20140925-1800
  YEdit Feature 1.0.16  org.dadacoalition.yedit.feature.group   YEdit Project
java -version
java version "1.8.0_25"
Java(TM) SE Runtime Environment (build 1.8.0_25-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode)
uname -a
Linux wks002 3.13.0-39-generic #66-Ubuntu SMP Tue Oct 28 13:30:27 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
@oyse
Copy link
Owner

oyse commented Nov 10, 2014

Thanks for reporting!

This is most probably not an easy thing to fix. The formatter is built on the DumperOptions from SnakeYAML which was not made with editor formatting in mind.

I don't have time to look at this at the moment, but I will try to take a look at in a couple of months.

@Andrei-Pozolotin
Copy link
Author

cool. thanks for looking.

@fchen
Copy link

fchen commented Oct 10, 2015

+1 for this request. At least auto-formatter should be able to act on only selected lines (if no lines are selected, then format all) so I can manually do partial formatting.

@oyse
Copy link
Owner

oyse commented Oct 11, 2015

As mentioned this is not an easy fix. The 3rd party parser that is used is not really made with this type of scenario in mind.

Suggestions for how this can be implemented or pull-requests are always welcome though.

@gazal-k
Copy link

gazal-k commented Mar 1, 2016

+1

@chrisinmtown
Copy link

+1 and I humbly offer my opinion that a comment-nuking formatter is fatally flawed and should be removed from this plugin.

@oyse
Copy link
Owner

oyse commented May 20, 2016

@chrisinmtown Yeah, you are probably right.

@moezubair
Copy link

+1

1 similar comment
@stevewallcgi
Copy link

+1

@Andrei-Pozolotin
Copy link
Author

Øystein: @oyse
can you please agree with Andrey how to best handle this:
https://bitbucket.org/asomov/snakeyaml/issues/346/

@asomov
Copy link

asomov commented Jun 8, 2016

  1. First, let us clarify the problem.
    YAML 1.1/1.2 specification explicitly ignores any comments. Any attempts to keep the comments will make SnakeYAML non-spec-compliant
  2. Feel free to make a proposal to respect the comments in YAML 2.0 (the spec is under construction).
  3. It should be (probably) possible to respect comments in a fork of SnakeYAML. The YAML processing (http://yaml.org/spec/1.1/current.html#id859109) should be extended with the support for comments (events, nodes etc)
    This is a major exercise.
  4. My recommendation is to drop auto-formatting altogether because it does not belong here.
    Formatting of YAML is sensitive, it has many flavours, it is a subject for major irritation and misunderstanding.

SnakeYAML developer.

P.S. Please do not use human names when you talk about Open Source project. It is always a community. (SnakeYAML is definitely no one-man-project)

@kdvolder
Copy link
Contributor

kdvolder commented Jun 9, 2016

I humbly offer my opinion that a comment-nuking formatter is fatally flawed and should be removed from this plugin.

I wouldn't argue againts calling it 'flawed'. However simply removing it helps no-one and might actually hamper someone. Even a flawed formatter may be useful to someone (i.e. if I don't have comments in my file then its totally fine). So I'd actually argue against removing.

If you don't like what the formatter does, you can always simply refrain from using it.

If its removed... then all choice regards to using or not using it is removed from the user as well.

@kdvolder
Copy link
Contributor

kdvolder commented Jun 9, 2016

What might be easy to do... is have the formatter run a quick check to see if any comments are present in the file, and if so either:

  • do nothing (i.e. no formatting)
  • popup a confirmation message asking the user if its okay to 'nuke' the comments

@asomov
Copy link

asomov commented Jun 9, 2016

What might be easy to do... is have the formatter run a quick check to see if any comments are >present in the file, and if so either:

This is not easy. The formatter does not know if there are any comments. They are removed.

@kdvolder
Copy link
Contributor

kdvolder commented Jun 9, 2016

I mean the formatter = "the thing in the YEdit editor which grabs the text from the edit buffer and massages it somehow".

Clearly that 'formatter' has access to the text of the document in full and it shouldn't be hard to grep the text with some regexp to see if it contains a comment anywhere.

@oyse
Copy link
Owner

oyse commented Jun 9, 2016

@kdvolder Yes, that could be do-able. Good suggestion.

@oyse
Copy link
Owner

oyse commented Jun 9, 2016

@Andrei-Pozolotin Adding this to SnakeYAML or to the Yaml 2.0 spec is not easy. I think a better approach would be that someone made a general Yaml formatter that could be used both in YEdit and other places, but I am not aware of any such project unfortunatly.

@Andrei-Pozolotin
Copy link
Author

@oyse @asomov thank you guys. need to think again.

@oyse : how about adding an eclipse plugin preference checkbox "enable auto format", and then keeping it disabled by default? basically, its hard to kill Ctrl+Shift+F reflex for some people, hence it needs some assistance :-)

@tedepstein
Copy link

+1 for disabling the Ctrl+Shfit+F hotkey by default. We do this in our commercial product, which includes our open source, Yedit based editor for Swagger-OpenAPI.

+1 for leaving the formatter in the current code base. Even though we've disabled the hot key, it's still helpful to have the command available through the QuickAccess toolbar, as a way to convert JSON to more idiomatic YAML. Those JSON sources don't have YAML comments, so the formatter is useful and non-destructive when used that way.

+1 for adding text-based comment detection and a pop-up warning before proceeding with formatting. A nice improvement!

+1 for (eventually) amending the YAML spec and SnakeYAML implementation to remove the requirement of ignoring/discarding comments. I think that's a significant design flaw that disregards the practical needs of YAML editors, and the humans who use them.

My team is slammed right now, but as we have available bandwidth, we hope to contribute to some of the above.

tfesenko added a commit to ModelSolv/yedit that referenced this issue Oct 12, 2016
Source format removes all comments and fixing it is not an easy thing to
do. As a temporary measure, we remove the key binding to avoid
unexpected information loss for the users.
@sandinak
Copy link

sandinak commented Jul 2, 2019

Can I recommend allowing for an external formatter like the python editor does with pep8? Would give us the option to format things as needed for the particular app. I get that it's not handled in the spec .. but it's heavily used in things like Ansible and others for documenting things in line.

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

No branches or pull requests