search

kwelch / babel-plugin-captains-log

Babel plugin that injects helpful details into console statements
MIT License
85 stars 5 forks source link

Trim Appended Filename #17 #20

Open neilholcomb opened 6 years ago

neilholcomb commented 6 years ago

Add new option to use file names relative to the project root.

New option useFileNameRelativeToProjectRoot <-- naming things is hard open to a better name here.

Option is false by default to not alter existing behavior. Again open to changing this so that it is true is the default behavior. Or not even have an option here???

I have tested both by using babel-cli, as well as inside an ejected create react app...However more testing would be a good idea. I am also not familiar with testing babel plugins and could use some help/advice for adding test cases to this code.

kwelch commented 6 years ago

Added a line about testing. Additionally, I would prefer this was default and only behavior to fileName injection since the longer names would not be helpful I believe.

That being said as I write this, I am thinking of logs within node and a full path like this may be beneficial for opening as a link from within the terminal.

This may be a good time to be a little more opinionated.

As for other testing, linking this to another project is a good way to validate that it works properly. The other project could be a simple node script or an existing web script. I typically use my family feud game repo, for real testing.

neilholcomb commented 6 years ago

I have moved getFileName into its own file under utils. And added some basic tests for this method.

The relative to project root path is now the expected default behavior when using injectFileName

I have tested this against a couple of my own projects by using yarn link I would still recommend you test against your projects before merging.

kwelch commented 6 years ago

I will take a look at this with a project of mine at some point this weekend. Thanks

kwelch commented 6 years ago

Could you all add your self to the all-contributors?

kwelch commented 6 years ago

I haven't forgotten about this. It is chill month for me so I am a little slower than normal.

neilholcomb commented 6 years ago

No problem...when the time comes let me know if there is anything else I can do to help land this PR

kwelch commented 6 years ago

Is this something you need? If it works for you and you are hoping to use it. I am good with merging based on what I have seen.

neilholcomb commented 6 years ago

No, sadly I don't have access to babel in my day job :-( ....this was just an excercise for me to learn more about babel plugins. Merge at your leisure.

kwelch commented 6 years ago

I appreciate the change and I hope it helped with your learning exercise I appreciate it.

kwelch commented 6 years ago

I am trying this out but it is not working as I expected. I am playing with your branch. I will push some changes as I look at it.

peteringram0 commented 6 years ago

Is this likely to ever make it into the project at all? Thanks.

kwelch commented 6 years ago

I am still open to this feature. If you have a recommendation on the solution I would love some help solving it. Last time I worked on it, I ran into some issues.