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

Location field autofill #1060

Open
flip111 opened this issue Oct 8, 2023 · 5 comments · May be fixed by #1288
Open

Location field autofill #1060

flip111 opened this issue Oct 8, 2023 · 5 comments · May be fixed by #1288

Comments

@flip111
Copy link
Contributor

flip111 commented Oct 8, 2023

Would be nice if the location field could be filled as far as it is possible. One would have to run this command git ls-remote --get-url origin, if the remote is not origin then i would further not support automatic detection.

@f-f
Copy link
Member

f-f commented Oct 14, 2023

This goes together with #1056

@fsoikin
Copy link
Contributor

fsoikin commented Sep 7, 2024

I'm not sure I understand. At which point should it be autofilled? And what does "autofilled" mean in this context? Just use that information in lieu of missing config? Or actually update the config?

@f-f
Copy link
Member

f-f commented Sep 7, 2024

I was thinking about actually updating the config. Not sure how exactly that would work out though - the type of the publish section of the config looks like this:

spago/core/src/Config.purs

Lines 95 to 101 in c4c9641

type PublishConfig =
{ version :: Version
, license :: License
, location :: Maybe Location
, include :: Maybe (Array FilePath)
, exclude :: Maybe (Array FilePath)
}

So if we don't have a version and a license then it doesn't make sense to proceed further.
But if we have those already then we could populate the configuration (we already do plenty of that) with the info gathered from the git remote, possibly issuing a warning to the user about that

@fsoikin
Copy link
Contributor

fsoikin commented Sep 11, 2024

Ok, just to clarify: does "auto-filling" involve writing the config back to disk? Or only adding it in memory after the config is read?

Also: is there a particular reason for doing all that patching in JS? Without any more context, I would assume this kind of patching should be part of the codecs.

@f-f
Copy link
Member

f-f commented Sep 13, 2024

does "auto-filling" involve writing the config back to disk? Or only adding it in memory after the config is read?

Generally we write the config back to disk (so it persists) and also update the data in memory so that execution can move forward, but in this particular case I think we should just write to disk and add an error to the pile: the user still needs to commit the new config before publishing can happen.

Also: is there a particular reason for doing all that patching in JS? Without any more context, I would assume this kind of patching should be part of the codecs.

This is something that should be better documented in our "dev" docs (which right now is the CONTRIBUTING.md file, but we could also have a HACKING.md or ARCHITECTURE.md).

Basically we want to preserve formatting and comments in the yaml configs we write back to disk. We also want to reuse the JSON codecs to parse/deal with the yaml configs, which are not rich enough to store all that info.
So we read the yaml with the yaml library, keep a hold of the Document that it returns, and then go ahead and parse its JSON representation with the JSON codecs.
When we need to update the config on disk we then act on the Document in JS land, which preserves formatting and comments when updated with the API we use.
We could model all of this in PureScript land, but I don't think the additional type safety justifies the ton of work that it would entail.

@fsoikin fsoikin linked a pull request Sep 21, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants