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

refactor: project configuration get/set parity #1400

Merged
merged 5 commits into from
Mar 23, 2022

Conversation

scolsen
Copy link
Contributor

@scolsen scolsen commented Mar 22, 2022

This commit refactors our handling of project configuration
getters/setters in hopes of making it easier to ensure fields that have
getters have setters and vice versa, and to ensure field keys are
consistent across the Project.config and get-config commands.

This commit also makes these commands return XObjs for all calls,
instead of coercing certain things into strings and also adds getters
for fields that previously lacked them, e.g.:

(Project.get-config "libflags")
=> ("-lm")

I've maintained parity with existing behaviors to avoid breaking
changes, thus, this works as it did before this commit:

(Project.config "libflags" "-lfoo")
(Project.get-config "libflags")
=> ("-lm" "-lfoo")

though it could now arguably take a list as an argument to permit
complete one-liner replacements; one could still add a single flag and
the interface would be more functional:

   (Project.config "libflags"
     (cons "-lfoo" (Project.get-config "libflags")))
   => ("-lm" "-lfoo")
   (Project.config "libflags" ("-lbar" "-qux"))
   => ("-lbar" "-lqux")

But, again, I've retained the old behavior to avoid breaking existing
builds.

This also fixes a small issue where by setting the file path print
length to "full" still resulted in the "short" printing scheme.

This commit refactors our handling of project configuration
getters/setters in hopes of making it easier to ensure fields that have
getters have setters and vice versa, and to ensure field keys are
consistent across the Project.config and get-config commands.

This commit also makes these commands return XObjs for all calls,
instead of coercing certain things into strings and also adds getters
for fields that previously lacked them, e.g.:

    (Project.get-config "libflags")
    => ("-lm")

I've maintained parity with existing behaviors to avoid breaking
changes, thus, this works as it did before this commit:

    (Project.config "libflags" "-lfoo")
    (Project.get-config "libflags")
    => ("-lm" "-lfoo")

though it could now arguably take a list as an argument to permit
complete one-liner replacements; one could still add a single flag and
the interface would be more functional:

   (Project.config "libflags"
     (cons "-lfoo" (Project.get-config "libflags")))
   => ("-lm" "-lfoo")
   (Project.config "libflags" ("-lbar" "-qux"))
   => ("-lbar" "-lqux")

But, again, I've retained the old behavior to avoid breaking existing
builds.

This also fixes a small issue where by setting the file path print
length to "full" still resulted in the "short" printing scheme.
@scolsen scolsen requested a review from a team March 22, 2022 03:18
@scolsen
Copy link
Contributor Author

scolsen commented Mar 22, 2022

I should also note that we now better account for a notion of "read-only" fields:

鲤 (Project.config "load-stack" "foo")
[CONFIG ERROR] the configuration field load-stack is read-only

And not all fields have to have getters/setters either, some have neither -- again, I tried to keep things matching the current behavior, but we might want to make more fields accessible through "read-only" getters now (e.g. Project.files):

鲤 (Project.get-config "files")
[CONFIG ERROR] Project.get-config can't understand the key 'files at REPL:3:21.

Traceback:
  (Project.get-config "files") at REPL:3:1.

I accidentally pluralized these field keys, which broke some existing
tests. This commit fixes that issue by restoring the original names.
Another small fix to ensure we retain backwards compatibility with
existing code after recent configuration handling changes: pkg-config
and load-stack fields should return array values, not lists.
Copy link
Collaborator

@eriksvedang eriksvedang left a comment

Choose a reason for hiding this comment

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

Nice improvements!

src/Obj.hs Outdated Show resolved Hide resolved
src/Project.hs Outdated Show resolved Hide resolved
Moves the project configuration frontend code into its own module to
avoid cyclic deps between Project and Obj and keep the Obj module
clean.

I've also removed some noisy comments about "internal only"
project configuration fields.
@eriksvedang eriksvedang merged commit 55255d3 into carp-lang:master Mar 23, 2022
@eriksvedang
Copy link
Collaborator

Awesome!

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.

None yet

2 participants