[#333] Use key-value map for extra files #369

chshersh · 2019-11-04T20:33:08Z

Resolves #333
Resolves #360

It actually works! I tested with the file from org repo. And the tree displays perfectly! Also, Summoner.Shell module turned out to be redundant...

Resolves #333

hint-man

There is no place for me here... I will choose the truth I like.

vrom911

This is great work, thanks @chshersh ❤️

vrom911 · 2019-11-05T05:54:05Z

stack-nightly.yaml

@@ -7,7 +7,6 @@ packages:
 extra-deps:
  - base-noprelude-4.13.0.0
  - brick-0.50
-  - relude-0.6.0.0


🎉 we are in the nightly!

vrom911 · 2019-11-05T05:56:09Z

stack.yaml

@@ -6,6 +6,7 @@ packages:

 extra-deps:
  - optparse-applicative-0.15.0.0
+  - relude-0.6.0.0


I left 0.5.0 intentionally here, so we check with both versions, as our boundaries are including it, do you think that it is not important? Should we then update the boundaries in the cabal file?

I've updated the boundaries in the .cabal file. I decided to bump up the lower bound because I'm using mapMaybeM function from a new version (it's very helpful 🎉) and summon simply won't compile with relude-0.5.0 😞 In general I don't mind supporting both versions but here I had to increase lower bound...

Oh, now I see!
You can put "Resolves #360 " then 🙂

vrom911 · 2019-11-05T06:09:09Z

summoner-cli/src/Summoner/Project.hs

+{- | This function fetches contents of extra file sources.
+-}
+fetchSources :: Bool -> Map FilePath Source -> IO [TreeFs]
+fetchSources isOffline = mapMaybeM sourceToTree . Map.toList


🎇 mapMaybeM 🎇

vrom911 · 2019-11-05T06:10:45Z

summoner-cli/src/Summoner/Project.hs

+        fetchSource isOffline source >>= \case
+            Nothing -> do
+                errorMessage $ "Error fetching: " <> toText path
+                pure Nothing


Is this correct, that in case of download (or just file getting) failure the process of the project generation will still continue without those files?

@vrom911 I'm not sure, but implementing different logic is quite complicated. For example, if isOffile is True then all Url will fail. But maybe this is intentional and maybe I don't want things like CONTRIBUTING.md while developing locally 🤔

Yes, makes sense. By the way, where the mentioned case is handled?

It's handled here:

summoner/summoner-cli/src/Summoner/Source.hs

Lines 65 to 67 in 38d1fce

Url url -> if isOffline

then Nothing <$ infoMessage ("Ignoring fetching from URL in offline mode from source: " <> url)

else fetchUrl url `catch` urlError url

Oh, right! Thank you 🍷

vrom911 · 2019-11-05T06:24:03Z

summoner-cli/src/Summoner/Tree.hs

+    (File _ _, Dir _ _) -> x : insertTree node xs
+    (File nodePath _, File curPath _)
+        | nodePath == curPath -> node : xs
+        | otherwise -> x : insertTree node xs


I don't quite get this case. When this file is already in the tree, it inserts it, but if it's not, it somehow continues inserting on the next layer? How so?

Right, the implementation is not obvious, but I tried to do my best. This function always inserts node to the list, at least when it reaches the empty list at the end:

insertTree node [] = [node]

In this case, if this function finds two Files with the same path, it will replace current x with node that why it's node : xs. This is done if people want to override default files like .travis.yml or Prelude.hs with their files. If file paths are not equal, then we keep x, and we will try to insert node into xs. Eventually, we either reach the end of the list, and we will add node unconditionally, or we will find a File that has the same name and will replace it.

Hmm, but are other nodes placed unconditionally? I thought there is some kind of hierarchy so no need to go till the end of the list each time.

Well, this list is not sorted at this stage. It could be [File "foo" x, File "bar" y] or [File "bar" x, File "foo" y]. I checked the implementation, and it looks like we sort this list only when printing to terminal or in TUI.

Now, after you told this, I started to think that probably there's a better way to implement TreeFs. At least TreeFs shouldn't have duplicate filepaths but since we're using lists, it's possible to have a bug in the implementation and introduce duplicate fields.

Maybe instead of

data TreeFs = Dir FilePath [TreeFs] | File FilePath Text

we should have

data TreeFs = Dir (Map FilePath TreeFs) | File Text

The latter representation won't allow having duplicate filepaths. But I believe that the current algorithm is also correct, even if the logic is not straightforward... And changing this type is a big rewrite...

Thank you for the clarification, Dmitrii! Yeah, I don't think that rewrite is the right step right now, and we could keep this function as it is for this PR. But let's think more about this sorting before option and create the issue if we will decide that it could work better and would become easier to maintain.

I've created the issue:

[RFC] Change the structure of TreeFs? #371

vrom911

Great! Thanks a lot 👏
Now, let the testing period begin!

* [#333] Use key-value map for extra files Resolves #333 * Fix stack

[#333] Use key-value map for extra files

9519637

Resolves #333

chshersh added feature config TOML configurations enhancement labels Nov 4, 2019

chshersh requested a review from vrom911 as a code owner November 4, 2019 20:33

chshersh self-assigned this Nov 4, 2019

hint-man bot approved these changes Nov 4, 2019

View reviewed changes

Fix stack

6f8bf23

chshersh force-pushed the chshersh/333-Use-key-value-map-for-extra branch from 790f95b to 6f8bf23 Compare November 4, 2019 21:10

vrom911 reviewed Nov 5, 2019

View reviewed changes

vrom911 approved these changes Nov 5, 2019

View reviewed changes

vrom911 merged commit c10b447 into map Nov 5, 2019

vrom911 deleted the chshersh/333-Use-key-value-map-for-extra branch November 5, 2019 07:30

chshersh added a commit that referenced this pull request Nov 5, 2019

[#333] Use key-value map for extra files (#369)

e12819e

* [#333] Use key-value map for extra files Resolves #333 * Fix stack

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#333] Use key-value map for extra files #369

[#333] Use key-value map for extra files #369

chshersh commented Nov 4, 2019 •

edited

Loading

hint-man bot left a comment

vrom911 left a comment

vrom911 Nov 5, 2019

vrom911 Nov 5, 2019

chshersh Nov 5, 2019

vrom911 Nov 5, 2019

vrom911 Nov 5, 2019

vrom911 Nov 5, 2019

chshersh Nov 5, 2019

vrom911 Nov 5, 2019

chshersh Nov 5, 2019

vrom911 Nov 5, 2019

vrom911 Nov 5, 2019

chshersh Nov 5, 2019

vrom911 Nov 5, 2019

chshersh Nov 5, 2019

vrom911 Nov 5, 2019

chshersh Nov 5, 2019

vrom911 left a comment

	Url url -> if isOffline
	then Nothing <$ infoMessage ("Ignoring fetching from URL in offline mode from source: " <> url)
	else fetchUrl url `catch` urlError url

[#333] Use key-value map for extra files #369

[#333] Use key-value map for extra files #369

Conversation

chshersh commented Nov 4, 2019 • edited Loading

hint-man bot left a comment

Choose a reason for hiding this comment

vrom911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrom911 left a comment

Choose a reason for hiding this comment

chshersh commented Nov 4, 2019 •

edited

Loading