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

Changed out path for overridden targets for consistency #1637

Merged
merged 2 commits into from
Dec 22, 2021

Conversation

lefou
Copy link
Member

@lefou lefou commented Dec 21, 2021

To be more consistent with the changes from PR
#1588
which re-layouted the log, meta, and dest path for targets.
This change touches the path of overriden target results.
It replaces the ../target/overriden/.. part with
../target.overriden/...

There is a catch though, to make this work I had to remove one
require assertion, which guarded the presence of dots (.) in
labels. TBH, I previously stubled upon this require
and also added the FIXME comment to it, as it is unclear why it
even exists. Probably, because labels with dots can't be properly
parsed when given on cli. More details and hisory can be found in
issue #820.

Fix #820.

Review by @lihaoyi.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 21, 2021

It would take some refactoring to make the path/segment handling "clean", but I'm fine with removing the assert for now

@lihaoyi
Copy link
Member

lihaoyi commented Dec 21, 2021

The fundamental issue with that assert is that Segment currently (with that assert) models things which can be parsed from the CLI, whereas the .override things cannot be parsed from the CLI. Thus using Segment to model the foo.override/ paths is not correct.

One slightly-less-hacky thing we could do is to add a third case to Segment, e.g. Segment.Override or something, to explicitly model this. Then we can ensure that the parser only returns Segment.Label or Segment.Cross, but other places we can synthesize Segment.Overrides as well. That would also be a good direction to go in if we in future decided to allow people to build the overridden targets directly from the CLI, which AFAIK is currently not allowed.

There are probably other things we could do, but this was the solution I thought of off the top of my head.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 21, 2021

On second thoughts, the .overridden data can only apply once to an entire Segments, and thus should apply to Segments as a whole and not be a replacement for any individual Segment

@lefou lefou changed the title Changed out path for overriden targets for consistency Changed out path for overridden targets for consistency Dec 21, 2021
@lefou
Copy link
Member Author

lefou commented Dec 21, 2021

@lihaoyi Nice thing is, this PR actually fixed #820.

@lefou
Copy link
Member Author

lefou commented Dec 21, 2021

I'd like to keep the mapping to Segment.Label as-is for now. CLI parsing is at some degree unrelated, as the removed require was just an after-the-fact guard and there is still a unique mapping of parsables to segments. The only issue is, that it's not bidirectional, but we don't generate cli arguments from segments, do we? (If we do, e.g. when completing, we could filter them out.) We could come up with some quoting or escaping, if someone really needs that feature, which is rather unlikely.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 22, 2021

@lefou we actually do generate a string from the Segments data structure in a few places via Segments#render, e.g. to populate the mill-profile.json.

I don't think we actually need it to be parseable - in the end it's just for debugging purposes - so I'm happy to merge this PR as is and figure out a way to clean things up later

To be more consistent with the changes from PR
com-lihaoyi#1588
which re-layouted the log, meta, and dest path for targets.
This change touches the path of overriden target results.
It replaces the `../target/overriden/..` part with
`../target.overriden/..`.

There is a catch though, to make this work I had to remove one
`require` assertion, which guarded the presence of dots (`.`) in
labels. TBH, I previously stubled upon this require
and also added the FIXME comment to it, as it is unclear why it
even exists. Probably, because labels with dots can't be properly
parsed when given on cli. More details and hisory can be found in
issue com-lihaoyi#820.

Fix com-lihaoyi#820.

Review by @lihaoyi
@lefou lefou merged commit a7d8956 into com-lihaoyi:main Dec 22, 2021
@lefou lefou deleted the overriden-out-path branch December 22, 2021 07:27
@lefou lefou added this to the after 0.10.0-M5 milestone Dec 22, 2021
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.

AssertionError when using predefScript.sc
2 participants