Fixes #36912 - support multi-line type definitions #367
Conversation
| @@ -24,6 +24,16 @@ module Kafo | |||
| it { _(parser.types).must_equal({'Ipv4' => 'Pattern[/^(\d+)\.(\d+)\.(\d+)\.(\d+)$/]'}) } | |||
| end | |||
|
|
|||
| describe "parse multiline alias" do | |||
| let(:file) { "type IP = Variant[\n IPv4,\n IPv6,\n]" } | |||
| it { _(parser.types).must_equal({'IP' => 'Variant[IPv4,IPv6,]'}) } | |||
There was a problem hiding this comment.
I wonder if those trailing comma will break something 😁
There was a problem hiding this comment.
added a test to ensure that it's fine
There was a problem hiding this comment.
What if we use puppet-strings to parse it? There's this part in the JSON:
"data_type_aliases": [
{
"name": "Candlepin::LogLevel",
"file": "types/loglevel.pp",
"line": 2,
"docstring": {
"text": "",
"tags": [
{
"tag_name": "summary",
"text": "A log4j log level"
}
]
},
"alias_of": "Enum['ALL', 'DEBUG', 'INFO', 'WARN', 'ERROR', 'FATAL', 'OFF', 'TRACE']"
}
],In #343 I started to utilize the bulk parsing approach. Current kafo_parsers calls puppet strings for every file. The bulk parsing approach is to call it for multiple files (or even for a whole module at once) and then extract the data from that file.
Since the data type aliases are also in there, we could leverage that and avoid writing our own parser.
lib/kafo/data_type_parser.rb
Outdated
| line = line.force_encoding("UTF-8").strip | ||
| next if line.start_with?('#') | ||
|
|
||
| line = line.split('#').first.strip |
There was a problem hiding this comment.
This will break if you use a Pattern with # in it.
There was a problem hiding this comment.
So will the current regex, no?
kafo/lib/kafo/data_type_parser.rb
Line 5 in 4b44b57
Ah, it ensures at least one space before the #…
There was a problem hiding this comment.
However, https://www.puppet.com/docs/puppet/7/lang_comments doesn't say anything about leading spaces
There was a problem hiding this comment.
added a test for the one Pattern I could find using a # inside, Stdlib::Email :)
This sounds like the correct approach long term, but it also seems like a bigger re-work is required to get that going correctly? |
evgeni
force-pushed
the
i36912
branch
2 times, most recently
from
4928690 to
a90cc79
Compare
There was a problem hiding this comment.
This sounds like the correct approach long term, but it also seems like a bigger re-work is required to get that going correctly?
Probably. The patches were written quite some time ago, but never really finished. It did give a massive speed up in building the installer so perhaps it's the excuse I need to properly wrap it up.
But short term this at least allows us to proceed.
lib/kafo/data_type_parser.rb
Outdated
|
|
||
| line = line.split(' #').first.strip | ||
| if line =~ TYPE_DEFINITION | ||
| lines << last_line |
There was a problem hiding this comment.
What's the idea behind the last_line here? When is that ever relevant for a type definition?
There was a problem hiding this comment.
Well, it's a buffer where I store all the lines in one string to drop the newlines.
I renamed the variable now, to make it more obvious
This strips line breaks from type definitions, thus making them parseable again. This will probably horribly break if type manifests would contain anything else but type definitions, but Puppet recommends [1] to store them in separate files and only have one type alias per file. [1] https://www.puppet.com/docs/puppet/7/lang_type_aliases
|
Let's see what theforeman/foreman-installer@66f3669 does in CI. |
|
it didn't explode
|
This strips line breaks from type definitions, thus making them parseable again.
This will probably horribly break if type manifests would contain anything else but type definitions, but Puppet recommends [1] to store them in separate files and only have one type alias per file.
[1] https://www.puppet.com/docs/puppet/7/lang_type_aliases