Skip to content

Updating the way we print names in the Cli Fixes #1155#2462

Merged
dubee merged 2 commits intoapache:masterfrom
jessealva:Fix-1155
Sep 21, 2017
Merged

Updating the way we print names in the Cli Fixes #1155#2462
dubee merged 2 commits intoapache:masterfrom
jessealva:Fix-1155

Conversation

@jessealva
Copy link
Contributor

Still working on getting the tests passing.

@jessealva
Copy link
Contributor Author

@dubeejw please review for comments so far

@jessealva jessealva changed the title WIP Fix for 1155 WIP Updating the way we print names in the Cli Fixes #1155 Jul 6, 2017
@jessealva
Copy link
Contributor Author

Fixes #1155

@jessealva
Copy link
Contributor Author

PG 3. #826

@jessealva
Copy link
Contributor Author

PG1, #1767 for after a merge with master that contained changes i had to merge manually.

@jessealva jessealva changed the title WIP Updating the way we print names in the Cli Fixes #1155 Updating the way we print names in the Cli Fixes #1155 Jul 12, 2017

import common.TestHelpers
import common.TestUtils
import common._
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't import everything from common unless we actually use everything in that package.

{
"id": "{{.ok}} deleted action {{.name}}\n",
"translation": "{{.ok}} deleted action {{.name}}\n"
"translation": "{{.ok}} deleted action '{{.name}}'\n"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to quote entity names that are displayed in bold font. Here is what it looks like:
ok: created action 'actionName'

@mdeuser, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Also looks like the keys (id) will be out of sync with the values (translation).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Any existing emphasis should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I agree that the keys will be out of sync, but I think its weird and wrong they have the translation string as the key to begin with. Thats unnecessary and confusing in my opinion, just didnt want to go up and change them all to some other string which is more easily searchable.

At the same time, boldness does not show on tools like Jenkins, travis, etc. Only on the CLI, so there could still be a reason to quote them @mdeuser @dubeejw

Although if you guys still think no, i can remove the quoting from the bolded strings.

@rabbah rabbah added the cli label Jul 23, 2017
@jessealva jessealva force-pushed the Fix-1155 branch 2 times, most recently from 3cf2f5b to 28ba975 Compare August 2, 2017 17:03
@jessealva
Copy link
Contributor Author

PG 1, 1932. @dubeejw @mdeuser

@rabbah
Copy link
Member

rabbah commented Sep 8, 2017

is this done @dubeejw @mdeuser?
we'd need to resolve conflicts to merge it.

@mdeuser
Copy link
Contributor

mdeuser commented Sep 14, 2017

@jessealva - can you rebase and issue another pg. out of the changes to 10 output strings, only 1 test was impacted...might indicate a need for an additional test or two (separate issue though)

@jessealva
Copy link
Contributor Author

jessealva commented Sep 15, 2017

When rebasing, had to fix about 20 different failures across the 5 files. Think most of these were covered.
I see what your saying. After finally merging this right, I see only the SDK tests are affected, so theres definitely some strings we're not testing for.

@jessealva
Copy link
Contributor Author

@mdeuser PG1, 2069. Looks clean now

@jessealva
Copy link
Contributor Author

ugh, not clean, looking into this again

@jessealva jessealva force-pushed the Fix-1155 branch 2 times, most recently from 54b903b to d21061b Compare September 19, 2017 23:17
@jessealva
Copy link
Contributor Author

PG2, 2095 @mdeuser

Copy link
Contributor

@mdeuser mdeuser left a comment

Choose a reason for hiding this comment

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

LGTM

dubee
dubee previously requested changes Sep 20, 2017
Copy link
Member

@dubee dubee left a comment

Choose a reason for hiding this comment

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

These files contain {{.name}} field that is not enclosed with single quotes:
action.go
api.go
package.go
trigger.go
rule.go
activations.go

This file contains {{.namespace}} field not enclosed in single qoutes:
namespace.go

Assuming all those occurrences are in bold.

@dubee
Copy link
Member

dubee commented Sep 20, 2017

PG2 2099 🔵

@dubee dubee merged commit 69c53aa into apache:master Sep 21, 2017
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Sep 22, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Sep 22, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 14, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 14, 2017
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
…2462)

* Changes so that we are more consistent with quoting names that are not bolded when we print statements

* Fixing up tests related to making quoting consistent in our CLI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants