Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

rewrite formatters not handling paths correctly and adding extra white space #173

Closed
ChrisMBarr opened this issue Jul 8, 2016 · 13 comments
Milestone

Comments

@ChrisMBarr
Copy link
Contributor

ChrisMBarr commented Jul 8, 2016

The formatters referenced in the readme file seem be described as actually modifying the source code of the passed in TypeScript files. This seems to be a little different than the original design for the formatters. Is this correct?

I have tried, but I cannot get these formatters to work the way they are described. I am using gulp-tslint and my basic code is below

var plumber = require("gulp-plumber");
var tsLint = require("gulp-tslint");

gulp.task("ts-fix", function () {
  return gulp.src(["app/**/*.ts"])
    .pipe(plumber())
    .pipe(tsLint({
      formattersDirectory: "./node_modules/tslint-microsoft-contrib",
      formatter: "fix-no-var-keyword"
    }))
    .pipe(tsLint.report());
});

in my tslint.json file the no-var-keyword rule is enabled. I do get failure messages about the var keyword being used, but no code is changed and the message that this formatter should log to the console is not present.

Am I misunderstanding something, or do I not have this set up incorrectly?

@HamletDRC
Copy link
Member

I use them from Grunt. They should work though, your configuration looks good. What I do to debug my build is open up ./node_modules/tslint-microsoft-contrib/fixNoVarKeywordFormatter.js on disk and start adding console.log statements to confirm that the formatter is actually found.

@ChrisMBarr
Copy link
Contributor Author

ChrisMBarr commented Jul 11, 2016

Good idea. I tried that and my logs were not output to the console! The paths are correct... perhaps this is an issue with gulp-tslint?

Should I ask about this on the main tslint project?

@HamletDRC
Copy link
Member

I wonder if verbose mode will help you.
I would try adding some console output to the gulp plugin and see why your formatter is not being invoked. Perhaps it is an issue with the formatterDirectory property.

@HamletDRC
Copy link
Member

Can I close this issue?

@ChrisMBarr
Copy link
Contributor Author

ChrisMBarr commented Jul 13, 2016

Actually it just so happens that gulp-tslint was updated to v6 which now supports custom formatters - I'm surprised to learn that it didn't before!
https://github.com/panuhorsmalahti/gulp-tslint/blob/master/CHANGELOG.md#600-2016-07-09

Now it does find the file and prints out my console logs, however I am getting errors from the formatter. I have a file located at C:/my-project/app/states/file.ts
my gulpfile.js and the /app/ directory are at the root of the /my-project/ folder and I use the glob pattern "app/**/*.ts" for TSLint.

However now when I run the gulp task I get this error:

Plumber found unhandled error:
 Error: ENOENT: no such file or directory, open 'C:\my-project\states\file.ts'

Notice how the app directory is not present in the path? I did some digging into the formatter and it looks like the fileName variable on line 15 only returns the relative path AFTER what matches in the glob pattnern, like states\file.ts. This means it doesn't have a full path to the file to be able to open the file and read it's contents.

Since I only plan to use this formatter once to do the initial fix I was able to get around this by temporarily changing my glob pattern and running the task. I just thought you might want to know.

@ChrisMBarr
Copy link
Contributor Author

Hmm... it also seems to change var into letr instead of let for some reason. Here's a screenshot of my diff after I let the task run.

image

It did this for every file. I'll have to revert the changes and attempt this formatter again in the future when it has been fixed 😫

@ChrisMBarr
Copy link
Contributor Author

ChrisMBarr commented Jul 13, 2016

Perhaps lines 19 & 20 need to be changed to this

const leftSide: string = fileContents.substring(0, start.getPosition() + 1);
const rightSide: string = fileContents.substring(start.getPosition() + 4);

Otherwise in the string var = 4;it replaces va with let and results in letr = 4;

I did that in my local copy and it seems to work in most cases, but not all....

@HamletDRC
Copy link
Member

I'm very sorry to hear that these formatters are not working for you. I wrote them for MSE and they worked there with no issues but clearly we have some different configuration. I am going to change the title of this and mark it as a bug so that we fix it.

@HamletDRC
Copy link
Member

I wonder if some of the spacing issues are line ending issues. What are the line endings of your files?

@HamletDRC HamletDRC changed the title How to use the custom formatters? rewrite formatters not handling paths correctly and adding extra white space Jul 14, 2016
@HamletDRC
Copy link
Member

The fomatters assume that your file is UTF-8 encoded. Is that the case for you? What is your file encoding?

@ChrisMBarr
Copy link
Contributor Author

We use "Windows line endings" and autocrlf = true is set in out .gitconfig file.

And yes, it appears we also use UTF-8 encoding.

Perhaps a better way to go about replacing the string would be to use a regex like the fixPreferConstFormatter. Find the actual text you want to replace instead of replacing whatever text exists at a certain index.

Perhaps: leftSide = leftSide.replace(/var(\s)*$/, 'let$1');

@HamletDRC HamletDRC added this to the 2.0.10 milestone Aug 11, 2016
@HamletDRC
Copy link
Member

Thanks for the patience here @ChrisMBarr

I fixed the formatter to handle different line feeds.

As for the path issue... I don' t know how I could fix this. The failure object only contains so much information.

@ChrisMBarr
Copy link
Contributor Author

Cool, thanks!

Yeah, I guess there's not much that you can do about the path. My workaround was to use a special glob pattern so it would be correct and exclude things in directories I didn't want it to happen in like node_modules and such. For me it was just a temp thing, I ran it once and then reverted my changes in the gulpfile since now everything is fixed and the rule is turned on to enforce it on any new changes.

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

No branches or pull requests

2 participants