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

Structured text language support #2310

Closed
wants to merge 10 commits into from
Closed

Structured text language support #2310

wants to merge 10 commits into from

Conversation

Serhioromano
Copy link
Contributor

As discussed in #1923 here is PR

@Serhioromano
Copy link
Contributor Author

Serhioromano commented Apr 12, 2020

I've solved conflicts. But that does not look intuitive. I thought the build process can run npx gulp and generate those files.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Thank you @Serhioromano for this PR!

I left you a few comments with things to improve.
And please fix the indentation of your language definition and test files. We only use tabs.

Regarding the merge conflict:
Seems like you fork wasn't up to date. To fix this:

  1. Update your fork and merge these changes into this branch
  2. Revert all changes you made to prism-show-language.js
  3. Run npx gulp

components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
components/prism-iecst.js Outdated Show resolved Hide resolved
Serhioromano and others added 6 commits April 12, 2020 20:15
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
@Serhioromano
Copy link
Contributor Author

I do not know why checks fail. It does work for local when I run npm run test****

@RunDevelopment
Copy link
Member

What is npm run test****?
If you run npm test you should see this:

  2 failing
  1) Patterns of 'iecst'
       - should have a capturing group if lookbehind is set to true:
     Error: Token <languages>.iecst.type.pattern: The pattern is set to 'lookbehind: true' but does not have a capturing group.
      at forEachPattern (tests/pattern-tests.js:124:10)
      at Context.<anonymous> (tests/pattern-tests.js:184:3)
  2) Patterns of 'iecst'
       - should not have unused capturing groups:
     Error: Token <languages>.iecst["class-name"]: Unused capturing group (PROGRAM|CONFIGURATION|INTERFACE|FUNCTION_BLOCK|FUNCTION|ACTION|TRANSITION|TYPE|STRUCT|(?:INITIAL_)?STEP|NAMESPACE|LIBRARY|CHANNEL|FOLDER|RESOURCE|VAR_(?:GLOBAL|INPUT|PUTPUT|IN_OUT|ACCESS|TEMP|EXTERNAL|CONFIG)|VAR|METHOD|PROPERTY). All capturing groups have to be either referenced or used as a Prism lookbehind group.
      at forEachPattern (tests/pattern-tests.js:124:10)
      at Context.<anonymous> (tests/pattern-tests.js:261:3)

@Serhioromano
Copy link
Contributor Author

This is how I run tests

Snag_ea3efa

@Serhioromano
Copy link
Contributor Author

By reading your messages I applied some changes. I didn't know where to see those messages.

return;
}

if (!Prism.plugins.toolbar) {
console.warn('Show Languages plugin loaded before Toolbar plugin.');

Copy link
Member

Choose a reason for hiding this comment

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

Please revert these changes. While the file might need a cleanup, it shouldn't be part of this PR.

The only change to this file should be this:

 		"ichigojam": "IchigoJam",
+		"iecst": "Structured Text (IEC 61131-3)",
 		"inform7": "Inform 7",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How? when I run npx gulp it rebuild this file and make them all one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yet, I am not sure how to revert a single file change. Out of my knowledge. I'll try to google it.

Copy link
Member

Choose a reason for hiding this comment

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

The easiest way would probably to copy-paste the content of the current prism-show-language.js in master and run npx gulp.

There's probably also some fancy git-way of doing it, but I'm not a git expert...

@RunDevelopment
Copy link
Member

This is how I run tests

This only checks your language against your .test files. We also have a few other tests which scan all patterns for potential errors (like the unnecessary lookbehind: true).
npm test runs all tests for all languages and basically verifies that Prism is fully functional.

By reading your messages I applied some changes. I didn't know where to see those messages.

What I showed you was the build log of Travis CI. To see this, click on the red X behind a commit on GitHub, click Details, choose any red (failed) build job, scroll waaaaay down.

@Serhioromano
Copy link
Contributor Author

I'll create a new PR. This copy of prism was forked years ago and propbably rebase did not work well.

@RunDevelopment
Copy link
Member

New PR: #2311

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

Successfully merging this pull request may close these issues.

None yet

2 participants