noseglid / atom-build

:hammer: Build your project directly from the Atom editor
https://atom.io/packages/build
MIT License
248 stars 97 forks source link

Support .atom-build.cson #178

Closed texastoland closed 9 years ago

texastoland commented 9 years ago

Would be especially nice for error regexes:

cmd: "jade"
args: ["index.jade", "--pretty"]
errorMatch: [
  ///
    \w+Error
    :\s (?<file> [^:]+ )
    :   (?<line>   \d+ )
  ///
]

Compare:

{
  "cmd": "jade",
  "args": ["index.jade", "--pretty"],
  "errorMatch": ["\\w+Error: (?<file>[^:]+):(?<line>\\d+)"]
}
noseglid commented 9 years ago

Thanks for the suggestion. I have a solution to this pushed to a branch; cson. I'd appreciate any feedback you might have.

texastoland commented 9 years ago

Looks solid I can't wait to try!

texastoland commented 8 years ago

Unfortunately the parser doesn't support regex :crying_cat_face: The only solution I can think would be to require CoffeeScript directly. PR posted by maintainer!

texastoland commented 8 years ago

@noseglid could you try updating cson-parser to ^1.3.0? @jkrems implemented it :tada:

noseglid commented 8 years ago

Cool. Sure thing

noseglid commented 8 years ago

I've updated to 1.3.0 on branch upgrade-cson-parser, however I don't think it will match your expectations. CSON-parser appears to interpret whatever is between two instances of /// as a regex, which is not true in a javascript sense. build uses XRegExp, which makes use of named matching groups (the (?<name>) syntax). This is not supported by native regex.

texastoland commented 8 years ago

You're right the generated JS regex itself crashes. XRegExp actually supports a lot of the same niceties but is still messy without tagged strings nor highlighted. Thinking of a solution.

texastoland commented 8 years ago

Ah you're requireing the JSON so tagged strings might be the answer! Trying it out…

texastoland commented 8 years ago

I opened an issue with XRegExp for one possible solution. Another using native JS would require passing along flags. Passing the object alone as if it were a RegExp doesn't work:

{
  cmd: "jade",
  args: ["index.jade", "--pretty"],
  errorMatch: [
    {
      pattern: `
        \\w+Error
        :\\s (?<file> [^:]+ )
        :    (?<line>   \\d+ )
      `,
      flags: "x"
    }
  ]
}
keplersj commented 8 years ago

If you want to use native regex syntax then we should look into handling regex like text-mate (the Atom package which does syntax highlighting) does.

texastoland commented 8 years ago

@noseglid Here are a couple alternatives to support nice regexes:

  1. Optionally pass flags with an ES6 tagged string
  2. Manually replace invalid capture tokens
keplersj commented 8 years ago

@dnalot Have you seen the way Atom CSON grammars are written?

texastoland commented 8 years ago

I've written them (1, 2). This ticket is about formatting configs to support extended regular expressions. I think a grammar would be overkill?

keplersj commented 8 years ago

I understand what this ticket is about. I'm suggesting that if you want to use vanilla CoffeeScript Regex syntax, you should follow by it's rules. Which would mean no named capture groups. If you wish to use CoffeeScript Regex Syntax then you should give the numbered capture group and identifier build can understand. See attaching number capture groups to identifiers here.

I suggest a syntax such as:

cmd: "jade"
args: ["index.jade", "--pretty"]
errorMatch: [
  {
    match: ///
      \w+Error
      :\s ([^:]+)
      :   (\d+)
    ///
    patterns:
      1: 'file'
      2: 'line' 
  }
]
texastoland commented 8 years ago

Brilliant! I tracked slevithan/xregexp#120 to allow {{name}} to work with XRegExp.build without associated patterns. Regardless XRegExp.build does appear far more flexible and would resolve passing flags with ES6 tagged strings, my issue with CSON named captures, and my XRegExp proposal if implemented :+1: @kepler0 Update this line?

var pattern = r.pattern || r // r is pattern when r.pattern is missing
var captures = r.captures // optional
var flags = r.flags // optional
return XRegExp.build(pattern, captures, flags)
keplersj commented 8 years ago

I'm not comfortable forcing named Capture groups on a Regex implementation that doesn't support them.

texastoland commented 8 years ago

I see what you mean now. My code still allows you to pass flags which you can't currently?

keplersj commented 8 years ago

A field for regex flags seems reasonable.

texastoland commented 8 years ago

Moreover XRegExp.build directly supports captures without special casing implementation or convoluting simple use cases. Here it is in action.

noseglid commented 8 years ago

Hey. Brilliant discussion. I suggest the following changes:

  1. Support for setting the regex flags
  2. Name the match groups in another entry.

So the following would be valid a valid .atom-build.json (of course it could be written in CoffeeScript as well):

{
  "cmd": "jade",
  "args": ["index.jade", "--pretty"],
  "errorMatch": [
  {
    "match": "\\w+Error:\\s ([^:]+):   (\\d+)",
    "flags": "m",
    "patterns": [ "file", "line" ]
  }
]

error-matcher.js must see the match entry in each errorMatch and if it is a regex, take it as it is, if it is a string, create a new regex with flags.

error-matcher.js must also see the absence of patterns entry, and in that case use XRegExp (for backward compatability).

Since arrays are ordered, this means that the first match group will be the file, and the second match group will be the line. Specifying the patterns entry as [ "line", "file" ] and it would be the other way around. Flags are the normal flags available in JavaScript regexes.

Does this make sense and suit all our needs?

texastoland commented 8 years ago

@noseglid Did you see my implementation/tests? It's exactly your proposal (I think) but always uses XRegExp (builtin build) with minimal implementation/maintenance cost.

texastoland commented 8 years ago

Also XRegExp always defers to RegExp under the hood if provided.