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

Add .resource extension to Robot Framework #6500

Merged

Conversation

Noordsestern
Copy link
Contributor

@Noordsestern Noordsestern commented Aug 4, 2023

Added missing file extension for Robot Framework resource files.

Description

Keyword-only files in Robot Framework have .resource extension since Robot Framework 3.1 (which is since a couple of years.). See User Guide with the Note The .resource extension is new in Robot Framework 3.1.

Checklist:

@Noordsestern Noordsestern requested a review from a team as a code owner August 4, 2023 06:54
@Nixinova
Copy link
Contributor

Nixinova commented Aug 4, 2023

.resource seems quite a generic extension. Are there any other languages using a .resource extension that this might be confused with? Those would need to be added in this PR too. Have a look through https://github.com/search?type=code&q=NOT+is%3Afork+path%3A*.resource - how many are Robot and how many aren't?

@Noordsestern
Copy link
Contributor Author

There does not seem to be another language, but as it is generic, there seem to be common files named that way in JS, CSS and XML. should i include the filenames for these languages in the PR? or better not, as it might mess something up?

@Alhadis Alhadis changed the title add .resource extension to Robot Framework Add .resource extension to Robot Framework Aug 14, 2023
@Alhadis
Copy link
Collaborator

Alhadis commented Aug 14, 2023

@Noordsestern Is there a particular pattern we can search for that unambiguously identifies .resource files used by the Robot Framework? If so, we could add .resource as a generic extension, which doesn't run the risk of misclassifying unrelated files, but does require an accurate and reliable heuristic be included.

@Noordsestern
Copy link
Contributor Author

Noordsestern commented Aug 15, 2023

Yes, there is a pattern. Content wise, they will certainly always contain at least 1 of the following lines:

*** Settings ***
*** Keywords ***
*** Variables ***

Due to the localization extension of Robot Framework, the lines could be translated. Maybe a regex would due, then.

Would that be sufficient?

@Alhadis
Copy link
Collaborator

Alhadis commented Aug 19, 2023

Due to the localization extension of Robot Framework, the lines could be translated. Maybe a regex would due, then.

Would that be sufficient?

I'm afraid not; those strings could easily appear in an unrelated file that ends in .resource; plus, the localisation extension you speak of would diminish such a heuristic's reliability anyway.

@Noordsestern
Copy link
Contributor Author

There are thousands of files. I checked first 5 pages of all appearances of the three lines in .resource files and they were all Robot Framework:

It seems very Robot Framework specific.

As for the localization feature: I would not consider it. When I localize Java reserved tokens, I would not be surprised if syntax highlighting breaks. Thus, I would localisation out.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

See inline comments

@Alhadis
Copy link
Collaborator

Alhadis commented Aug 31, 2023

See inline comments

Where?

@lildude
Copy link
Member

lildude commented Aug 31, 2023

I was referring to your comments @Alhadis as I can't mark as "request changes" without leaving a comment 😁

@Noordsestern
Copy link
Contributor Author

I am a bit lost 😰 What is requested change exactly?

I cannot proof that there won't be a .resource file that

  • has a line like ``*** Settings | Variables | Keywords ***`
  • and is not Robot Framework.

The search results suggest, that it is extremely likely that it is Robot Framework when above conditions are met.

I am eager to comply, but I do not understand what change is required.

@F3licity
Copy link

See inline comments

I don't see any inline comments either. One often needs to submit his review for the comments to appear. Maybe you need to double check that you completed your reviewing ?

@lildude
Copy link
Member

lildude commented Sep 22, 2023

I don't see any inline comments either. One often needs to submit his review for the comments to appear. Maybe you need to double check that you completed your reviewing ?

As I said in response to @Alhadis's comment about this, I was referring to his comments as I couldn't mark the PR as needing changes without making a comment... so I did complete my review, just poorly worded it 😁.

I've taken a closer look at the search results for those strings and I think we should be OK with using them as a specific enough heuristic for this generic extension for Robot. Like you, I can't find any files containing those specific strings which don't appear to be Robot (from my naïve understanding of Robot files 😁); I can find plenty which definitely aren't Robot without them, hence we have to go the generic route.

So the next steps are to add a heuristic and add an entry to the generic.yml file. You can look at the PRs for one of the other extensions in that file for an example of what needs to be done.

@Noordsestern
Copy link
Contributor Author

How is the popularity being measured? What kind of popularity is meant?

There are plenty of hits when searching for a most common combination of .resource files for Robot Framework.

Robot Framework is popular for open source test automation with 2,6 million downloads per month.

It is common topic on testing conferences, is backed by foundation, , is popular on the open source realm for 15 years now. There are tons of ecosystem projects . And keyword files happen to have the suffix .resource (see guides).

Also take in to account, that use case for Robot Framework test automation (it also used for RPA, but that use case is less popular). Teams that use Robot Framework in big scale work for usualyl for enterprises, thus their test repositories are not public. You may find them even more often in enterprise github (that's why i opened this PR, since the syntax highlighting is missing in my enterprise repositories).

What else is needed to pass popularity threshold?

@lildude
Copy link
Member

lildude commented Mar 15, 2024

How is the popularity being measured? What kind of popularity is meant?

I added the label based purely on the search in the OP.

We measure popularity as detailed in #5756 as referenced in the CONTRIBUTING.md file.

lildude
lildude previously approved these changes Jun 7, 2024
@lildude lildude dismissed their stale review June 7, 2024 10:22

Whoops. Looks like you've got test failures

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Please address the issue that is causing the test failures

Copy link
Contributor Author

@Noordsestern Noordsestern left a comment

Choose a reason for hiding this comment

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

there seems to have been a typo. fixed.

@Noordsestern Noordsestern requested a review from lildude August 6, 2024 20:25
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Needs a heuristic test still.

@lildude lildude added this pull request to the merge queue Aug 31, 2024
Merged via the queue into github-linguist:master with commit 15e5607 Aug 31, 2024
5 checks passed
@Noordsestern Noordsestern deleted the Noordsestern-missing-robot-extension branch September 5, 2024 09:12
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Dec 20, 2024
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.

6 participants