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

allow usage of extname(N) template in custom labels #213033

Merged

Conversation

bsShoham
Copy link
Contributor

@bsShoham bsShoham commented May 19, 2024

This pull requests add to the custom label service the option to accept a new template variable in the patterns, which allows to insert into the label each of the parts of the full file name (including the extension) separated by .

usage example:

settings.json:

    "workbench.editor.customLabels.enabled": true,
    "workbench.editor.customLabels.patterns": {
        "**/unit-tests/**/*.spec.*": "Test ${filenamePart(1)}",
        "**/e2e/**/*.e2e.spec.*": "Test ${filenamePart(2)} ${filenamePart(1)}",
    }

The results would be:
WORKSPACE/units-tests/users.spec.ts -> Test users
WORKSPACE/e2e/users.e2e.spec.ts -> Test e2e users

fixes #210166

also fixes some small spelling mistakes.

@bsShoham
Copy link
Contributor Author

@bsShoham please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@bsShoham bsShoham force-pushed the dev/add-filename-part-custom-label-template branch from c441332 to 3ce177e Compare May 19, 2024 12:49
@benibenj
Copy link
Contributor

Thank you for creating a PR for this. I think it makes sense to tackle this case.
I see you want to introduce a variable filenamePart. I wonder if this might be a bit confusing.

I would propose the following:

  • filename: name of the file without any extension
  • extname: the entire extension
  • extname(N): a specific part of the extension starting from the back

Example path: src/main.ex1.ex2.ex3

  • filename: main
  • extname: ex1.ex2.ex3
  • extname(0): ex3
  • extname(1): ex2
  • extname(2): ex1
  • extname(3): This does not work and would show the template (${extname(3)}) to indicate to the user that his pattern/template is invalid

@bsShoham bsShoham force-pushed the dev/add-filename-part-custom-label-template branch from 3ce177e to ed1e1da Compare May 29, 2024 10:08
@benibenj benibenj added this to the June 2024 milestone May 29, 2024
@benibenj benibenj self-requested a review May 29, 2024 10:53
@bsShoham
Copy link
Contributor Author

@benibenj Thanks for the reply and the suggestions!
I've gone and made changes according to your proposal, would love it if you can have a look.

@benibenj
Copy link
Contributor

Thanks for making the changes!
We are currently preparing for the release next week (this means no more changes can go in) so I'll review this in June and we can make it part of the June release

@benibenj benibenj changed the title allow usage of filenamePart template in custom labels allow usage of extname(N) template in custom labels Jun 15, 2024
@benibenj
Copy link
Contributor

benibenj commented Jun 15, 2024

Heii @bsShoham, PR looks mostly good. What I've noticed:

  • ${extname} doesn't work anymore. Should this maybe just return the entire extension? (file.txt1.txt2 => txt1.txt2)
  • When I use an N which is larger then the available extensions it should render ${extname(N)} to indicate to the user that his template has a mistake, however it currently shows just extname(N). (Checkout what happens when you use ${dirname(100)})
image
  • ${filename} use to include all extensions except the last one, we did this because we did not have any support for ${extname(N)} but as we do support it with this PR, it should probably only include the name of the file without an extension. What do you think?

@bsShoham
Copy link
Contributor Author

bsShoham commented Jun 16, 2024

@benibenj Hi, thanks for the reply and the comments, fixed the match return at N larger than extensions amount, and repaired to regex to fix extname.


As far as the behavior for filename, after seeing some comments and other issues, I believe that there are 3 major use cases we would like to support.

The normal case

SomeComponent.tsx => SomeComponent

I believe this is the most common use case, no need to debate there.

The hidden / 3rd party tool files

.gitignore / .eslint.json => .gitignore / .eslint or gitignore / eslint

This is a more complex case, of files that start with ..
I believe we would like to support both of the options above, while being opinionated about which one is through filename
and the other could be using extname. I'm inclined towards keeping the . prefix, though it can go both ways.

The tests

userSignIn.spec.ts - > userSignIn / userSignIn.spec

Also a debatable case, though not as much, since capturing those through pattern (src/**/*.spec.ts) is a more favorable solution,
I believe we should exclude the extension and keep the filename without any extensions (userSignIn)


I kept the filename as is in the meanwhile, I'll add the changes for it after the discussion is done.

@benibenj
Copy link
Contributor

benibenj commented Jun 17, 2024

First of all, I think the following should always hold:

  • original filename = ${filename}.${extname}

I think filename should work in the following way:

  • SomeComponent.tsx => SomeComponent
  • .eslint.json => .eslint
  • userSignIn.spec.ts => userSignIn

.eslint.json and .gitignore are tricky ones, but I think including the dot would make sense, however it would not be possible to map .someFile.json to someFile.json in this case. Only way we could support it is if ${extname} => json but ${extname(-1)} / ${extname(2)} => someFile but this seems a bit weird. Might be fine to not support it from the start and wait to see if there is a common use case for it

@bsShoham bsShoham force-pushed the dev/add-filename-part-custom-label-template branch from 7980403 to a58d188 Compare June 17, 2024 08:50
@benibenj
Copy link
Contributor

@bsShoham I added test so we can make sure the behaviour works as described

@bsShoham bsShoham force-pushed the dev/add-filename-part-custom-label-template branch from 431d255 to 138f183 Compare June 18, 2024 09:58
have filename return the file name from the start until the first non-leading `.`
@bsShoham
Copy link
Contributor Author

@benibenj
Done with the filename changes and made sure all tests pass
Let me know if you can think of other cases I should verify

@benibenj benibenj enabled auto-merge (squash) June 20, 2024 15:22
@benibenj
Copy link
Contributor

benibenj commented Jun 20, 2024

Thanks @bsShoham for working on this!

@benibenj benibenj merged commit 4d858a6 into microsoft:main Jun 20, 2024
6 checks passed
@bsShoham bsShoham deleted the dev/add-filename-part-custom-label-template branch June 25, 2024 08:08
bricefriha pushed a commit to bricefriha/vscode that referenced this pull request Jun 26, 2024
* allow usage of `filenamePart` template in custom labels

* use extname(n) instead of filename part

* change extname to return the full extensions and not just the last file extension

* Fix regex to match extname
Return match when N in `extname(N)` is larger than extensions amount

* Add tests

* Fix `.file` label assertion

* Make sure `extname` doesn't include leading dots
have filename return the file name from the start until the first non-leading `.`

* 💄

---------

Co-authored-by: BeniBenj <besimmonds@microsoft.com>
Co-authored-by: Benjamin Christopher Simmonds <44439583+benibenj@users.noreply.github.com>
@Ardian-Hoti
Copy link

Ardian-Hoti commented Jul 4, 2024

great very nice, for nested ext , but what about nested folders and dynamic folders in nextjs for example , like i have folders

src/app/(dashboard)/
        users/
           [username]/
             page.tsx
        page.tsx

    and probably more nested folders like 5-6 folders deep how to customize these dynamic folders 

@benibenj
Copy link
Contributor

benibenj commented Jul 4, 2024

@Ardian-Hoti similar to ${extname(N)} we also support ${dirname(N)}

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.

Custom Labels: improve ${extname} when file includes multiple extensions
4 participants