Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

(feature): FixFormatter: new formatter for printing fixed files #2149

Closed
wants to merge 2 commits into from

Conversation

ianks
Copy link

@ianks ianks commented Jan 28, 2017

PR checklist

  • New feature, bugfix, or enhancement
    • Includes tests

What changes did you make?

This PR adds a FixFormatter which take a single file, and prints the fixed contents of said file. The goal is to be able to integrate with editor plugins such as neoformat, which rely on the the formatted text being printed to stdout.

Currently, this formatter will only work when passing in a single file. I think from a practicality standpoint, this makes a lot of sense as the output will always be merged into a single stream (stdout, some file, etc), where the end user probably will not want to merge all of the files together.

Happy to any suggestions and improve this in any way you see fit!

-Ian

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @ianks! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@nchen63 nchen63 left a comment

Choose a reason for hiding this comment

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

sorry, I thought about this more, and I don't think it will work because the fixes from different rules could overwrite each other. Our internal logic actually reloads the file after each rule applies its fixes to avoid this issue. Could you work around this by applying the fixes on a copy of the file and using that as your input?

@nchen63 nchen63 closed this Jan 29, 2017
@ianks
Copy link
Author

ianks commented Jan 29, 2017

@nchen63 why was this closed?

@nchen63
Copy link
Contributor

nchen63 commented Jan 29, 2017

The approach of using a formatter just isn't feasible given my reasons above.

@ianks
Copy link
Author

ianks commented Jan 29, 2017

Ok, so what would be the best way to get this feature implemented? Should we continue the discussion in #2125?

@nchen63
Copy link
Contributor

nchen63 commented Jan 29, 2017

sure, let me know if my idea of copying the file doesn't work for you

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants