Improve deleting a src file UX #24

googleson78 · 2022-04-14T12:39:11Z

Fixes #22

googleson78 · 2022-04-14T12:40:05Z

gazelle_haskell_modules/lang.go

+
+func cleanupHiddenModulesList(r *rule.Rule, ruleNameSet map[string]bool) {
+	ruleName := r.Name()
+	// TODO: use something better?


Not sure if this isn't already implemented somewhere around here, but I didn't find it.

googleson78 · 2022-04-14T12:40:11Z

gazelle_haskell_modules/lang.go

 	f.Sync()
 }

+func cleanupModulesList(r *rule.Rule, ruleNameSet map[string]bool) {
+	// TODO: use labels instead of manually stripping away the ':' ?


Is there a better way to do this?

googleson78 · 2022-04-14T14:16:42Z

TODO: add fix invocation to CI

EDIT: done, I think

We need to not crash early on missing files, so that fix can handle them later.

This has two results * we don't crash when a src for a haskell_module is missing * the haskell_module is deleted in Fix, since we no longer get the data in ruleInfos

When a module is deleted it's still referenced in the modules attribute of a rule. Here we manually remove it.

This is to be used for hidden_modules too. Also delete the list if it's left empty after deletion.

In there, running fix is required to get it to build.

googleson78 · 2022-04-15T08:33:15Z

himportscan/exe/Main.hs


 main :: IO ()
 main = do
  haskellSrcs <- lines <$> getContents
-  mapM scanImportsFromFile haskellSrcs >>= printImports
+  -- silently ignore missing files
+  -- TODO: it would be better to instead report them to the user


I would do this here, but I would need to alter the JSON format with which go and hs communicate, so I'd prefer this to be done in a separate PR.

gazelle_haskell_modules/rule_generation.go

facundominguez

Thanks @googleson78!

Very nice breakdown of commits.

I added some comments. The PR is already in pretty good shape though.

gazelle_haskell_modules/rule_generation.go

facundominguez · 2022-04-28T14:18:09Z

gazelle_haskell_modules/rule_generation.go

-			OriginatingRules: oRules,
-			ModuleData:       modDatas[0],
-		}
+		if len(modDatas) == 1 {


The weakest condition to run the then branch is len(modDatas) > 0. Maybe this if statement can be simplified to

if len(modDatas) > 0 { ... } else { log.Printf(...) }

I implemented this in b2f66f2

Thanks. I wonder if we should (in another issue perhaps) change the logic here to error if we receive len(modData) > 1, as an assert, since it shouldn't be possible?

I'd be fine with such a change.

I'll open an issue after we merge this PR.

facundominguez · 2022-04-28T14:55:00Z

gazelle_haskell_modules/lang.go

+		nonDeletedModules := make([]string, 0, len(modules))
+		for _, module := range modules {
+			// TODO: use labels instead of manually stripping away the ':' ?
+			if ruleNameSet[module[1:]] {


It is a bit dangerous to assume that the first character of the label is a colon. The user might try to add modules manually here, and we can't make assumptions about those. It is best to check explicitly that module[1] == ':' and keep the label if it has a different character.

module[1] != byte(':') || ruleNameSet[module[1:]]

In the latest version of the code perhaps something like this would do:

- stripColon := func(module string) string { return module[1:] } + stripColon := func(module string) string { + if strings.HasPrefix(module, ":") { + return module[1:] + } else { + return module + } + }

I tried a fix in 8e5ab77

Actually, I think that right now (and before the patch too), if a user writes a fully qualified bazel target in modules, it will get deleted, regardless of whether the module exists or not, because ruleNameSet won't have the fully qualified module name.

So there probably needs to be something more complex here - either store full target names in ruleNameSet, or do a more complex check here.

I think the fully qualified names might be a better solution, as this string manipulation stuff feels very brittle both here and for hidden_modules :/

8e5ab77

Seems to have broken CI - need to investigate.

I think the fully qualified names might be a better solution, as this string manipulation stuff feels very brittle both here and for hidden_modules :/

A question that helps determining the severity of issues in general is: what the best workaround is?
In light of this question, you are correct that absolute labels in modules can confuse the fix command, but it is also easy for the user to make the labels relative instead, and relative labels are the most economical to use anyway. So it is unlikely to be a barrier for adoption.

Seems to have broken CI - need to investigate.

Mea culpa 🙈
Fixed in efae1e7

And still I was dropping non-relative labels. I tried another fix in 2614494.

facundominguez · 2022-04-28T15:02:42Z

gazelle_haskell_modules/rule_generation.go

-			OriginatingRules: oRules,
-			ModuleData:       modDatas[0],
-		}
+		if len(modDatas) == 1 {


Perhaps for another PR: if the rule is marked with a # keep comment, probably it shouldn't be discarded here.

I'll open an issue after this PR is merged.

facundominguez · 2022-04-28T15:20:26Z

gazelle_haskell_modules/lang.go

+func fixHiddenModulesList(r *rule.Rule, ruleNameSet map[string]bool) {
+	ruleName := r.Name()
+	// TODO: use something better?
+	addPackageName := func(module string) string { return fmt.Sprintf(":%s.%s", ruleName, module) }


IIUC, you want to drop the colon from the output.

Actually, even fixing the above, this implementation is deleting all modules from hidden_modules which don't have a haskell_module rule in the same BUILD file.

Also, if a haskell_module rule is used in more than one library, this implementation will also delete the hidden module in one of them even if the haskell_rule hasn't been removed.

Perhaps we can improve both things in a later PR.

What do you mean by "output"? I'm adding this here so it matches the format in ruleNameSet. It seems a bit brittle though - is there some way to make it better?

I stripped the colon in 5496a59.

What do you mean by "output"? I'm adding this here so it matches the format in ruleNameSet. It seems a bit brittle though - is there some way to make it better?

Let me know if 5496a59 doesn't clarify these questions.

I'm not really sure why it works both with and without the colon (this code is out of cache right now 😅 ) but I trust your judgement :)

A first short answer is that ruleNameSet doesn't include the leading :, so it needs to be removed. Though note that the code has evolved since this conversation. Happy to engage with more details if needed.

gazelle_haskell_modules/lang.go

example/fix-cleanup-modules/BUILD.bazel

gazelle_haskell_modules/rule_generation.go

facundominguez · 2022-04-29T12:30:20Z

@googleson78, are there any other changes that you would like to have related to this PR?

…pModulesList

…t rules

Format go srcs

c5842f1

googleson78 commented Apr 14, 2022

View reviewed changes

googleson78 force-pushed the fix-deleted-file branch from 443cf2e to 1a9261e Compare April 14, 2022 14:16

googleson78 force-pushed the fix-deleted-file branch from 151c5cb to e33f781 Compare April 14, 2022 14:34

googleson78 added 9 commits April 15, 2022 11:01

Ignore missing files in himportscan

4b43a20

We need to not crash early on missing files, so that fix can handle them later.

Don't add missing haskell_module src data

4f0ec96

This has two results * we don't crash when a src for a haskell_module is missing * the haskell_module is deleted in Fix, since we no longer get the data in ruleInfos

Remove deleted modules from module lists in rules

c6f01b4

When a module is deleted it's still referenced in the modules attribute of a rule. Here we manually remove it.

Factor out common logic for fixing modules lists

66186d4

This is to be used for hidden_modules too. Also delete the list if it's left empty after deletion.

Also cleanup hidden_modules

0810562

Rename cleanup functions

133cf72

Mention new fix functionality in readme

6460213

Add an example with a deleted file

0354df1

In there, running fix is required to get it to build.

Run gazelle fix in ci

74b5172

googleson78 force-pushed the fix-deleted-file branch from e33f781 to 74b5172 Compare April 15, 2022 08:02

googleson78 commented Apr 15, 2022

View reviewed changes

gazelle_haskell_modules/rule_generation.go Outdated Show resolved Hide resolved

facundominguez reviewed Apr 28, 2022

View reviewed changes

facundominguez added 3 commits April 29, 2022 07:19

strip a leading colon only if present in stripColon

8e5ab77

Simplify if statement dealing with missing sources

b2f66f2

Enhance the error message when files are missing.

a49dd86

facundominguez reviewed Apr 29, 2022

View reviewed changes

gazelle_haskell_modules/rule_generation.go Show resolved Hide resolved

facundominguez and others added 3 commits April 29, 2022 08:28

Fix lookup of hidden modules when deleting rules

5496a59

Document the fix-cleanup-modules test case

268ba57

Fix index in stripColon

efae1e7

facundominguez added 3 commits April 29, 2022 10:49

Abstract the decision to keep labels/modules in a predicate in cleanu…

2614494

…pModulesList

Keep non-relative labels in cleanupModulesList

b4f1b0c

Only cleanup modules and hidden_modules in haskell_library/binary/tes…

15988d3

…t rules

facundominguez force-pushed the fix-deleted-file branch from e18e2de to 15988d3 Compare April 29, 2022 13:59

facundominguez merged commit 51bb487 into main Apr 29, 2022

facundominguez deleted the fix-deleted-file branch April 29, 2022 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve deleting a src file UX #24

Improve deleting a src file UX #24

googleson78 commented Apr 14, 2022

googleson78 Apr 14, 2022

googleson78 Apr 14, 2022

googleson78 commented Apr 14, 2022 •

edited

Loading

googleson78 Apr 15, 2022

facundominguez left a comment

facundominguez Apr 28, 2022

facundominguez Apr 29, 2022

googleson78 Apr 29, 2022

facundominguez Apr 29, 2022

googleson78 Apr 29, 2022

googleson78 Apr 29, 2022

facundominguez Apr 28, 2022 •

edited

Loading

facundominguez Apr 29, 2022

facundominguez Apr 29, 2022

googleson78 Apr 29, 2022

googleson78 Apr 29, 2022

facundominguez Apr 29, 2022

facundominguez Apr 29, 2022

facundominguez Apr 29, 2022 •

edited

Loading

facundominguez Apr 28, 2022

googleson78 Apr 29, 2022

googleson78 Apr 29, 2022

facundominguez Apr 28, 2022

facundominguez Apr 29, 2022

googleson78 Apr 29, 2022

facundominguez Apr 29, 2022

facundominguez Apr 29, 2022 •

edited

Loading

googleson78 Apr 29, 2022

facundominguez Apr 29, 2022

facundominguez commented Apr 29, 2022

Improve deleting a src file UX #24

Improve deleting a src file UX #24

Conversation

googleson78 commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 commented Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

facundominguez 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

facundominguez Apr 28, 2022 • edited Loading

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

facundominguez Apr 29, 2022 • edited Loading

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

facundominguez Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundominguez commented Apr 29, 2022

googleson78 commented Apr 14, 2022 •

edited

Loading

facundominguez Apr 28, 2022 •

edited

Loading

facundominguez Apr 29, 2022 •

edited

Loading

facundominguez Apr 29, 2022 •

edited

Loading